Re: [PATCH] clk: ti: omap5+: dpll: implement errata i810

[Stephen Boyd]

On 11/30, Tero Kristo wrote:
> Errata i810 states that DPLL controller can get stuck while transitioning
> to a power saving state, while its M/N ratio is being re-programmed.
> 
> As a workaround, before re-programming the M/N ratio, SW has to ensure
> the DPLL cannot start an idle state transition. SW can disable DPLL
> idling by setting the DPLL AUTO_DPLL_MODE=0 or keeping a clock request
> active by setting a dependent clock domain in SW_WKUP.
> 
> This errata impacts OMAP5 and DRA7 chips, so enable the errata for these.
> 
> Signed-off-by: Tero Kristo <t-kri...@ti.com>
> ---

Acked-by: Stephen Boyd <sb...@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] clk: ti: fixes for 4.4-rc

[Stephen Boyd]

On 11/24, Tero Kristo wrote:
> Hi Michael, Stephen,
> 
> Here are some TI clock driver fixes for 4.4-rc.
> 
> -Tero
> 
> 
> 
> The following changes since commit 8005c49d9aea74d382f474ce11afbbc7d7130bec:
> 
>   Linux 4.4-rc1 (2015-11-15 17:00:27 -0800)
> 
> are available in the git repository at:
> 
>   https://github.com/t-kristo/linux-pm.git for-4.4-rc/ti-clk-fixes
> 
> for you to fetch changes up to 167af5ef2cdba14ff14a13c91e5532ed479083d8:
> 
>   clk: ti: drop locking code from mux/divider drivers (2015-11-24
> 11:30:27 +0200)
> 

Thanks. Pulled into clk-fixes.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clk: ti: dra7: constify clk_hw_omap_ops structure

[Stephen Boyd]

On 11/15, Julia Lawall wrote:
> The clk_hw_omap_ops structures are never modified, so declare this one as
> const, like the others.
> 
> Done with the help of Coccinelle.
> 
> Signed-off-by: Julia Lawall 
> 
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clk: gpio: Get parent clk names already in of_gpio_clk_setup()

[Stephen Boyd]

On 11/17, Jyri Sarha wrote:
> Get parent clk names already in of_gpio_clk_setup() and store the
> names in struct clk_gpio_delayed_register_data. of_clk_get_parent_name()
> can not be called in struct of_clk_provider's get() callback since it
> may make a recursive call to of_clk_get_from_provider() and this in turn
> tries to recursively lock of_clk_mutex.
> 
> Signed-off-by: Jyri Sarha 
> Cc: Sergej Sawazki 
> ---
> Something has changed in Linux mainline so that getting the clk
> parent names in struct of_clk_provider's get() callback does not work
> anymore. This patch should fix the problem.

Something would be commit 0a4807c2f9a4 (clk: Make
of_clk_get_parent_name() robust with #clock-cells = 1, 2015-10-14)?

The patch looks good. Given that there's one user of this code in
mainline I'll push this as a fix for v4.4.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RESEND] ARM: Remove __ref on hotplug cpu die path

[Stephen Boyd]

Now that __cpuinit has been removed, the __ref markings on these
functions are useless. Remove them. This also reduces the size of
the multi_v7_defconfig image:

$ size before after
   textdata bss dec hex filename
   126835781470996  348904 14503478 dd4e36 before
   126832741470996  348904 14503174 dd4d06 after

presumably because now we don't have to jump to code in the
.ref.text section and/or the noinline marking is removed.

Cc: Tony Lindgren <t...@atomide.com>
Acked-by: Barry Song <bao...@kernel.org>
Acked-by: Andy Gross <agr...@codeaurora.org>
Acked-by: Viresh Kumar <vire...@kernel.org>
Cc: Shiraz Hashim <shiraz.linux.ker...@gmail.com>
Cc: Stephen Warren <swar...@wwwdotorg.org>
Acked-by: Thierry Reding <thierry.red...@gmail.com>
Cc: Alexandre Courbot <gnu...@gmail.com>
Acked-by: Linus Walleij <linus.wall...@linaro.org>
Acked-by: Sudeep Holla <sudeep.ho...@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
Cc: Will Deacon <will.dea...@arm.com>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: <linux-omap@vger.kernel.org>
Cc: <linux-arm-...@vger.kernel.org>
Cc: <spear-de...@list.st.com>
Cc: <linux-te...@vger.kernel.org>
Signed-off-by: Stephen Boyd <sb...@codeaurora.org>
---

Collected the acks and resent.

 arch/arm/kernel/psci_smp.c   | 4 ++--
 arch/arm/mach-omap2/omap-hotplug.c   | 2 +-
 arch/arm/mach-omap2/omap-wakeupgen.c | 2 +-
 arch/arm/mach-prima2/hotplug.c   | 2 +-
 arch/arm/mach-qcom/platsmp.c | 2 +-
 arch/arm/mach-realview/hotplug.c | 2 +-
 arch/arm/mach-spear/hotplug.c| 2 +-
 arch/arm/mach-tegra/hotplug.c| 2 +-
 arch/arm/mach-ux500/hotplug.c| 2 +-
 arch/arm/mach-vexpress/hotplug.c | 2 +-
 10 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
index 61c04b02faeb..9d479b2ea40d 100644
--- a/arch/arm/kernel/psci_smp.c
+++ b/arch/arm/kernel/psci_smp.c
@@ -71,7 +71,7 @@ int psci_cpu_disable(unsigned int cpu)
return 0;
 }
 
-void __ref psci_cpu_die(unsigned int cpu)
+void psci_cpu_die(unsigned int cpu)
 {
u32 state = PSCI_POWER_STATE_TYPE_POWER_DOWN <<
PSCI_0_2_POWER_STATE_TYPE_SHIFT;
@@ -83,7 +83,7 @@ void __ref psci_cpu_die(unsigned int cpu)
panic("psci: cpu %d failed to shutdown\n", cpu);
 }
 
-int __ref psci_cpu_kill(unsigned int cpu)
+int psci_cpu_kill(unsigned int cpu)
 {
int err, i;
 
diff --git a/arch/arm/mach-omap2/omap-hotplug.c 
b/arch/arm/mach-omap2/omap-hotplug.c
index 971791fe9a3f..593fec753b28 100644
--- a/arch/arm/mach-omap2/omap-hotplug.c
+++ b/arch/arm/mach-omap2/omap-hotplug.c
@@ -27,7 +27,7 @@
  * platform-specific code to shutdown a CPU
  * Called with IRQs disabled
  */
-void __ref omap4_cpu_die(unsigned int cpu)
+void omap4_cpu_die(unsigned int cpu)
 {
unsigned int boot_cpu = 0;
void __iomem *base = omap_get_wakeupgen_base();
diff --git a/arch/arm/mach-omap2/omap-wakeupgen.c 
b/arch/arm/mach-omap2/omap-wakeupgen.c
index db7e0bab3587..2da0d974f2f7 100644
--- a/arch/arm/mach-omap2/omap-wakeupgen.c
+++ b/arch/arm/mach-omap2/omap-wakeupgen.c
@@ -330,7 +330,7 @@ static int irq_cpu_hotplug_notify(struct notifier_block 
*self,
return NOTIFY_OK;
 }
 
-static struct notifier_block __refdata irq_hotplug_notifier = {
+static struct notifier_block irq_hotplug_notifier = {
.notifier_call = irq_cpu_hotplug_notify,
 };
 
diff --git a/arch/arm/mach-prima2/hotplug.c b/arch/arm/mach-prima2/hotplug.c
index 0ab2f8bae28e..a728c78b996f 100644
--- a/arch/arm/mach-prima2/hotplug.c
+++ b/arch/arm/mach-prima2/hotplug.c
@@ -32,7 +32,7 @@ static inline void platform_do_lowpower(unsigned int cpu)
  *
  * Called with IRQs disabled
  */
-void __ref sirfsoc_cpu_die(unsigned int cpu)
+void sirfsoc_cpu_die(unsigned int cpu)
 {
platform_do_lowpower(cpu);
 }
diff --git a/arch/arm/mach-qcom/platsmp.c b/arch/arm/mach-qcom/platsmp.c
index 5cde63a64b34..9b00123a315d 100644
--- a/arch/arm/mach-qcom/platsmp.c
+++ b/arch/arm/mach-qcom/platsmp.c
@@ -49,7 +49,7 @@ extern void secondary_startup_arm(void);
 static DEFINE_SPINLOCK(boot_lock);
 
 #ifdef CONFIG_HOTPLUG_CPU
-static void __ref qcom_cpu_die(unsigned int cpu)
+static void qcom_cpu_die(unsigned int cpu)
 {
wfi();
 }
diff --git a/arch/arm/mach-realview/hotplug.c b/arch/arm/mach-realview/hotplug.c
index ac22dd41b135..968e2d1964f6 100644
--- a/arch/arm/mach-realview/hotplug.c
+++ b/arch/arm/mach-realview/hotplug.c
@@ -90,7 +90,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int 
*spurious)
  *
  * Called with IRQs disabled
  */
-void __ref realview_cpu_die(unsigned int cpu)
+void realview_cpu_die(unsigned int cpu)
 {
int spurious = 0;
 
diff --git a/arch/arm/mach-spear/hotplug.c b/arch/arm/mach-spear/hotplug.c
index d97749c642ce..12edd1cf8a12 100644
--- a/

Re: [GIT PULL] clock: ti: fixes for 4.3-rc

[Stephen Boyd]

On 10/02, Tero Kristo wrote:
> Hi Stephen, Mike,
> 
> A few TI clock driver fixes to pull against 4.3-rc.
> 
> -Tero
> 
> 
> 
> The following changes since commit 9ffecb10283508260936b96022d4ee43a7798b4c:
> 
>   Linux 4.3-rc3 (2015-09-27 07:50:08 -0400)
> 
> are available in the git repository at:
> 
>   https://github.com/t-kristo/linux-pm.git for-4.3-rc/ti-clk-fixes
> 

Thanks, pulled. I guess we'll wait until Monday next week to send
off to Linus so this can go through a couple -next cycles.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clk: ti: dflt: fix enable_reg validity check

[Stephen Boyd]

On 09/29, Suman Anna wrote:
> diff --git a/drivers/clk/ti/clkt_dflt.c b/drivers/clk/ti/clkt_dflt.c
> index 90d7d8a21c49..1ddc288fce4e 100644
> --- a/drivers/clk/ti/clkt_dflt.c
> +++ b/drivers/clk/ti/clkt_dflt.c
> @@ -222,7 +222,7 @@ int omap2_dflt_clk_enable(struct clk_hw *hw)
>   }
>   }
>  
> - if (unlikely(!clk->enable_reg)) {
> + if (unlikely(IS_ERR(clk->enable_reg))) {

IS_ERR() already has an unlikely inside it, so the unlikely is
redundant here.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clk: ti: fix dual-registration of uart4_ick

[Stephen Boyd]

On 09/29, Ben Dooks wrote:
> On the OMAP AM3517 platform the uart4_ick gets registered
> twice, causing any power managment to /dev/ttyO3 to fail
> when trying to wake the device up.
> 
> This solves the following oops:
> 
> [] Unhandled fault: external abort on non-linefetch (0x1028) at 0xfa09e008
> [] PC is at serial_omap_pm+0x48/0x15c
> [] LR is at _raw_spin_unlock_irqrestore+0x30/0x5c
> 
> Cc: sta...@vger.kernel.org
> Cc: mturque...@baylibre.com
> Cc: sb...@codeaurora.org
> Cc: linux-...@vger.kernel.org
> Cc: linux-omap@vger.kernel.org
> Cc: t-kri...@ti.com
> Cc: linux-ker...@lists.codethink.co.uk
> Signed-off-by: Ben Dooks 

Which patch broke this? Adding a Fixes: line will help us figure
out where to backport this.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: Remove __ref on hotplug cpu die path

[Stephen Boyd]

Now that __cpuinit has been removed, the __ref markings on these
functions are useless. Remove them. This also reduces the size of
the multi_v7_defconfig image:

$ size before after
   textdata bss dec hex filename
   126835781470996  348904 14503478 dd4e36 before
   126832741470996  348904 14503174 dd4d06 after

presumably because now we don't have to jump to code in the
.ref.text section and/or the noinline marking is removed.

Cc: Tony Lindgren <t...@atomide.com>
Cc: Barry Song <bao...@kernel.org>
Cc: Andy Gross <agr...@codeaurora.org>
Cc: Viresh Kumar <vire...@kernel.org>
Cc: Shiraz Hashim <shiraz.linux.ker...@gmail.com>
Cc: Stephen Warren <swar...@wwwdotorg.org>
Cc: Thierry Reding <thierry.red...@gmail.com>
Cc: Alexandre Courbot <gnu...@gmail.com>
Cc: Linus Walleij <linus.wall...@linaro.org>
Cc: Sudeep Holla <sudeep.ho...@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
Cc: Will Deacon <will.dea...@arm.com>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: <linux-omap@vger.kernel.org>
Cc: <linux-arm-...@vger.kernel.org>
Cc: <spear-de...@list.st.com>
Cc: <linux-te...@vger.kernel.org>
Signed-off-by: Stephen Boyd <sb...@codeaurora.org>
---

This patch can be broken up into per-SoC if desired.

 arch/arm/kernel/psci_smp.c   | 4 ++--
 arch/arm/mach-omap2/omap-hotplug.c   | 2 +-
 arch/arm/mach-omap2/omap-wakeupgen.c | 2 +-
 arch/arm/mach-prima2/hotplug.c   | 2 +-
 arch/arm/mach-qcom/platsmp.c | 2 +-
 arch/arm/mach-realview/hotplug.c | 2 +-
 arch/arm/mach-spear/hotplug.c| 2 +-
 arch/arm/mach-tegra/hotplug.c| 2 +-
 arch/arm/mach-ux500/hotplug.c| 2 +-
 arch/arm/mach-vexpress/hotplug.c | 2 +-
 10 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
index 61c04b02faeb..9d479b2ea40d 100644
--- a/arch/arm/kernel/psci_smp.c
+++ b/arch/arm/kernel/psci_smp.c
@@ -71,7 +71,7 @@ int psci_cpu_disable(unsigned int cpu)
return 0;
 }
 
-void __ref psci_cpu_die(unsigned int cpu)
+void psci_cpu_die(unsigned int cpu)
 {
u32 state = PSCI_POWER_STATE_TYPE_POWER_DOWN <<
PSCI_0_2_POWER_STATE_TYPE_SHIFT;
@@ -83,7 +83,7 @@ void __ref psci_cpu_die(unsigned int cpu)
panic("psci: cpu %d failed to shutdown\n", cpu);
 }
 
-int __ref psci_cpu_kill(unsigned int cpu)
+int psci_cpu_kill(unsigned int cpu)
 {
int err, i;
 
diff --git a/arch/arm/mach-omap2/omap-hotplug.c 
b/arch/arm/mach-omap2/omap-hotplug.c
index 971791fe9a3f..593fec753b28 100644
--- a/arch/arm/mach-omap2/omap-hotplug.c
+++ b/arch/arm/mach-omap2/omap-hotplug.c
@@ -27,7 +27,7 @@
  * platform-specific code to shutdown a CPU
  * Called with IRQs disabled
  */
-void __ref omap4_cpu_die(unsigned int cpu)
+void omap4_cpu_die(unsigned int cpu)
 {
unsigned int boot_cpu = 0;
void __iomem *base = omap_get_wakeupgen_base();
diff --git a/arch/arm/mach-omap2/omap-wakeupgen.c 
b/arch/arm/mach-omap2/omap-wakeupgen.c
index e1d2e991d17a..d9b317706e63 100644
--- a/arch/arm/mach-omap2/omap-wakeupgen.c
+++ b/arch/arm/mach-omap2/omap-wakeupgen.c
@@ -330,7 +330,7 @@ static int irq_cpu_hotplug_notify(struct notifier_block 
*self,
return NOTIFY_OK;
 }
 
-static struct notifier_block __refdata irq_hotplug_notifier = {
+static struct notifier_block irq_hotplug_notifier = {
.notifier_call = irq_cpu_hotplug_notify,
 };
 
diff --git a/arch/arm/mach-prima2/hotplug.c b/arch/arm/mach-prima2/hotplug.c
index 0ab2f8bae28e..a728c78b996f 100644
--- a/arch/arm/mach-prima2/hotplug.c
+++ b/arch/arm/mach-prima2/hotplug.c
@@ -32,7 +32,7 @@ static inline void platform_do_lowpower(unsigned int cpu)
  *
  * Called with IRQs disabled
  */
-void __ref sirfsoc_cpu_die(unsigned int cpu)
+void sirfsoc_cpu_die(unsigned int cpu)
 {
platform_do_lowpower(cpu);
 }
diff --git a/arch/arm/mach-qcom/platsmp.c b/arch/arm/mach-qcom/platsmp.c
index 5cde63a64b34..9b00123a315d 100644
--- a/arch/arm/mach-qcom/platsmp.c
+++ b/arch/arm/mach-qcom/platsmp.c
@@ -49,7 +49,7 @@ extern void secondary_startup_arm(void);
 static DEFINE_SPINLOCK(boot_lock);
 
 #ifdef CONFIG_HOTPLUG_CPU
-static void __ref qcom_cpu_die(unsigned int cpu)
+static void qcom_cpu_die(unsigned int cpu)
 {
wfi();
 }
diff --git a/arch/arm/mach-realview/hotplug.c b/arch/arm/mach-realview/hotplug.c
index ac22dd41b135..968e2d1964f6 100644
--- a/arch/arm/mach-realview/hotplug.c
+++ b/arch/arm/mach-realview/hotplug.c
@@ -90,7 +90,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int 
*spurious)
  *
  * Called with IRQs disabled
  */
-void __ref realview_cpu_die(unsigned int cpu)
+void realview_cpu_die(unsigned int cpu)
 {
int spurious = 0;
 
diff --git a/arch/arm/mach-spear/hotplug.c b/arch/arm/mach-spear/hotplug.c
index d97749c642ce..12edd1cf8a12 100644
--- a/arch/arm/mach

[PATCH 2/2] clk: Convert __clk_get_name(hw-clk) to clk_hw_get_name(hw)

[Stephen Boyd]

Use the provider based method to get a clock's name so that we
can get rid of the clk member in struct clk_hw one day. Mostly
converted with the following coccinelle script.

@@
struct clk_hw *E;
@@

-__clk_get_name(E-clk)
+clk_hw_get_name(E)

Cc: Heiko Stuebner he...@sntech.de
Cc: Sylwester Nawrocki s.nawro...@samsung.com
Cc: Tomasz Figa tomasz.f...@gmail.com
Cc: Peter De Schrijver pdeschrij...@nvidia.com
Cc: Prashant Gaikwad pgaik...@nvidia.com
Cc: Stephen Warren swar...@wwwdotorg.org
Cc: Thierry Reding thierry.red...@gmail.com
Cc: Alexandre Courbot gnu...@gmail.com
Cc: Tero Kristo t-kri...@ti.com
Cc: Ulf Hansson ulf.hans...@linaro.org
Cc: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
Cc: Andrew Bresticker abres...@chromium.org
Cc: Ezequiel Garcia ezequiel.gar...@imgtec.com
Cc: Ralf Baechle r...@linux-mips.org
Cc: Kevin Cernekee cerne...@chromium.org
Cc: Geert Uytterhoeven geert+rene...@glider.be
Cc: Ulrich Hecht ulrich.hecht+rene...@gmail.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-rockc...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Cc: linux-te...@vger.kernel.org
Cc: linux-omap@vger.kernel.org
Signed-off-by: Stephen Boyd sb...@codeaurora.org
---
 drivers/clk/berlin/berlin2-pll.c |  4 ++--
 drivers/clk/clk-xgene.c  | 22 +++---
 drivers/clk/pistachio/clk-pll.c  |  4 ++--
 drivers/clk/qcom/clk-branch.c|  2 +-
 drivers/clk/rockchip/clk-inverter.c  |  2 +-
 drivers/clk/rockchip/clk-mmc-phase.c |  2 +-
 drivers/clk/samsung/clk-pll.c| 18 +-
 drivers/clk/shmobile/clk-div6.c  |  2 +-
 drivers/clk/st/clk-flexgen.c |  4 ++--
 drivers/clk/st/clkgen-fsyn.c | 18 +-
 drivers/clk/st/clkgen-mux.c  |  2 +-
 drivers/clk/st/clkgen-pll.c  |  8 
 drivers/clk/tegra/clk-pll.c  |  8 
 drivers/clk/ti/apll.c|  4 ++--
 drivers/clk/ti/clkt_dflt.c   |  8 
 drivers/clk/ti/clockdomain.c | 14 +++---
 drivers/clk/ux500/clk-prcmu.c| 16 
 drivers/clk/ux500/clk-sysctrl.c  |  2 +-
 18 files changed, 70 insertions(+), 70 deletions(-)

diff --git a/drivers/clk/berlin/berlin2-pll.c b/drivers/clk/berlin/berlin2-pll.c
index f4b8d324b083..1c2294d3ba85 100644
--- a/drivers/clk/berlin/berlin2-pll.c
+++ b/drivers/clk/berlin/berlin2-pll.c
@@ -61,7 +61,7 @@ berlin2_pll_recalc_rate(struct clk_hw *hw, unsigned long 
parent_rate)
fbdiv = (val  map-fbdiv_shift)  FBDIV_MASK;
rfdiv = (val  map-rfdiv_shift)  RFDIV_MASK;
if (rfdiv == 0) {
-   pr_warn(%s has zero rfdiv\n, __clk_get_name(hw-clk));
+   pr_warn(%s has zero rfdiv\n, clk_hw_get_name(hw));
rfdiv = 1;
}
 
@@ -70,7 +70,7 @@ berlin2_pll_recalc_rate(struct clk_hw *hw, unsigned long 
parent_rate)
vcodiv = map-vcodiv[vcodivsel];
if (vcodiv == 0) {
pr_warn(%s has zero vcodiv (index %d)\n,
-   __clk_get_name(hw-clk), vcodivsel);
+   clk_hw_get_name(hw), vcodivsel);
vcodiv = 1;
}
 
diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
index 4caee9356407..96a6190acac2 100644
--- a/drivers/clk/clk-xgene.c
+++ b/drivers/clk/clk-xgene.c
@@ -74,7 +74,7 @@ static int xgene_clk_pll_is_enabled(struct clk_hw *hw)
u32 data;
 
data = xgene_clk_read(pllclk-reg + pllclk-pll_offset);
-   pr_debug(%s pll %s\n, __clk_get_name(hw-clk),
+   pr_debug(%s pll %s\n, clk_hw_get_name(hw),
data  REGSPEC_RESET_F1_MASK ? disabled : enabled);
 
return data  REGSPEC_RESET_F1_MASK ? 0 : 1;
@@ -112,7 +112,7 @@ static unsigned long xgene_clk_pll_recalc_rate(struct 
clk_hw *hw,
fref = parent_rate / nref;
fvco = fref * nfb;
}
-   pr_debug(%s pll recalc rate %ld parent %ld\n, __clk_get_name(hw-clk),
+   pr_debug(%s pll recalc rate %ld parent %ld\n, clk_hw_get_name(hw),
fvco / nout, parent_rate);
 
return fvco / nout;
@@ -225,7 +225,7 @@ static int xgene_clk_enable(struct clk_hw *hw)
spin_lock_irqsave(pclk-lock, flags);
 
if (pclk-param.csr_reg != NULL) {
-   pr_debug(%s clock enabled\n, __clk_get_name(hw-clk));
+   pr_debug(%s clock enabled\n, clk_hw_get_name(hw));
reg = __pa(pclk-param.csr_reg);
/* First enable the clock */
data = xgene_clk_read(pclk-param.csr_reg +
@@ -234,7 +234,7 @@ static int xgene_clk_enable(struct clk_hw *hw)
xgene_clk_write(data, pclk-param.csr_reg +
pclk-param.reg_clk_offset);
pr_debug(%s clock PADDR base %pa clk offset 0x%08X mask 0x%08X 
value 0x%08X\n,
-   __clk_get_name(hw-clk), reg,
+   clk_hw_get_name(hw), reg,
pclk-param.reg_clk_offset

Re: [PATCH 05/12] ARM: OMAP2+: Add support for initializing dm814x clocks

[Stephen Boyd]

On 07/16/2015 02:37 AM, Tony Lindgren wrote:


Here's this patch updated with the above removed.



Ok. I fixed up Mike's email in case he wants to look at it. Looks fine 
to me though.




8 -
From: Tony Lindgren t...@atomide.com
Date: Thu, 16 Jul 2015 01:55:57 -0700
Subject: [PATCH] ARM: OMAP2+: Add support for initializing dm814x clocks

Let's add a minimal clocks for dm814x to get it booted. This is
mostly a placeholder and relies on the PLLs being on from the
bootloader.

Note that the divider clocks work the same way as on dm816x and
am335x.

Cc: Matthijs van Duin matthijsvand...@gmail.com
Cc: Mike Turquette mturque...@linaro.org
Cc: Paul Walmsley p...@pwsan.com
Cc: Stephen Boyd sb...@codeaurora.org
Cc: Tero Kristo t-kri...@ti.com
Signed-off-by: Tony Lindgren t...@atomide.com


Acked-by: Stephen Boyd sb...@codeaurora.org



--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -558,7 +558,7 @@ void __init ti814x_init_early(void)
ti81xx_hwmod_init();
omap_hwmod_init_postsetup();
if (of_have_populated_dt())
-   omap_clk_soc_init = ti81xx_dt_clk_init;
+   omap_clk_soc_init = dm814x_dt_clk_init;
  }
  
  void __init ti816x_init_early(void)

@@ -575,7 +575,7 @@ void __init ti816x_init_early(void)
ti81xx_hwmod_init();
omap_hwmod_init_postsetup();
if (of_have_populated_dt())
-   omap_clk_soc_init = ti81xx_dt_clk_init;
+   omap_clk_soc_init = dm816x_dt_clk_init;
  }
  #endif
  
--- a/drivers/clk/ti/Makefile

+++ b/drivers/clk/ti/Makefile
@@ -2,7 +2,7 @@ obj-y   += clk.o autoidle.o 
clockdomain.o
  clk-common= dpll.o composite.o divider.o gate.o \
  fixed-factor.o mux.o apll.o
  obj-$(CONFIG_SOC_AM33XX)  += $(clk-common) clk-33xx.o
-obj-$(CONFIG_SOC_TI81XX)   += $(clk-common) fapll.o clk-816x.o
+obj-$(CONFIG_SOC_TI81XX)   += $(clk-common) fapll.o clk-814x.o 
clk-816x.o
  obj-$(CONFIG_ARCH_OMAP2)  += $(clk-common) interface.o clk-2xxx.o
  obj-$(CONFIG_ARCH_OMAP3)  += $(clk-common) interface.o \
   clk-3xxx.o
--- /dev/null
+++ b/drivers/clk/ti/clk-814x.c
@@ -0,0 +1,31 @@
+/*
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ */
+
+#include linux/kernel.h
+#include linux/clk-provider.h
+#include linux/clk/ti.h
+
+static struct ti_dt_clk dm814_clks[] = {
+   DT_CLK(NULL, devosc_ck, devosc_ck),
+   DT_CLK(NULL, mpu_ck, mpu_ck),
+   DT_CLK(NULL, sysclk4_ck, sysclk4_ck),
+   DT_CLK(NULL, sysclk6_ck, sysclk6_ck),
+   DT_CLK(NULL, sysclk10_ck, sysclk10_ck),
+   DT_CLK(NULL, sysclk18_ck, sysclk18_ck),
+   DT_CLK(NULL, timer_sys_ck, devosc_ck),
+   DT_CLK(NULL, cpsw_125mhz_gclk, cpsw_125mhz_gclk),
+   DT_CLK(NULL, cpsw_cpts_rft_clk, cpsw_cpts_rft_clk),
+   { .node_name = NULL },
+};
+
+int __init dm814x_dt_clk_init(void)
+{
+   ti_dt_clocks_register(dm814_clks);
+   omap2_clk_disable_autoidle_all();
+   omap2_clk_enable_init_clocks(NULL, 0);
+
+   return 0;
+}
--- a/drivers/clk/ti/clk-816x.c
+++ b/drivers/clk/ti/clk-816x.c
@@ -42,7 +42,7 @@ static const char *enable_init_clks[] = {
ddr_pll_clk3,
  };
  
-int __init ti81xx_dt_clk_init(void)

+int __init dm816x_dt_clk_init(void)
  {
ti_dt_clocks_register(dm816x_clks);
omap2_clk_disable_autoidle_all();
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -329,7 +329,8 @@ int ti_clk_add_component(struct device_node *node, struct 
clk_hw *hw, int type);
  int omap3430_dt_clk_init(void);
  int omap3630_dt_clk_init(void);
  int am35xx_dt_clk_init(void);
-int ti81xx_dt_clk_init(void);
+int dm814x_dt_clk_init(void);
+int dm816x_dt_clk_init(void);
  int omap4xxx_dt_clk_init(void);
  int omap5xxx_dt_clk_init(void);
  int dra7xx_dt_clk_init(void);



--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL]

[Stephen Boyd]

On 07/15, Tero Kristo wrote:
 Hi Stephen, Mike,
 
 Here is the updated pull-request based on Stephen's request. This is
 on top of 4.1-rc2. The branch is force updated version of the one
 Stephen tried yesterday, the old one contained the minor build
 issues present on v3 of the patch set.
 
 I did a quick try of merging this branch on top of 4.2-rc2 and it
 works fine without any conflicts.
 

Thanks. Pulled into clk-next.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] clk: ti: clock driver code migration to drivers

[Stephen Boyd]

On 07/14/2015 01:54 AM, Tero Kristo wrote:
The following changes since commit 
bc0195aad0daa2ad5b0d76cce22b167bc3435590:


  Linux 4.2-rc2 (2015-07-12 15:10:30 -0700)


Why did this get rebased onto v4.2-rc2? I thought it was all ready to go 
based on v4.1-rc2? If possible leave it on v4.1-rc2 please.




are available in the git repository at:

  g...@github.com:t-kristo/linux-pm.git for-4.3/ti-clk-move


Also I can't fetch this. Please give a public URL and not the one you 
use for github.


I fetched the for-4.2/ti-clk-move branch and did a test merge and fixed 
up omap3_noncore_dpll_determine_rate() and 
omap4_dpll_regm4xen_determine_rate() for the new determine rate 
signature and it looks like things are still compiling. So please redo 
the tag for that branch.


8-

diff --cc include/linux/clk/ti.h
index 448b4f87b9eb,f1838256fbaa..
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
diff --git a/drivers/clk/ti/clock.h b/drivers/clk/ti/clock.h
index d4d232fd89bc..d8aafd333058 100644
--- a/drivers/clk/ti/clock.h
+++ b/drivers/clk/ti/clock.h
@@ -250,12 +250,8 @@ int omap3_noncore_dpll_set_rate_and_parent(struct clk_hw 
*hw,
   unsigned long rate,
   unsigned long parent_rate,
   u8 index);
-long omap3_noncore_dpll_determine_rate(struct clk_hw *hw,
-  unsigned long rate,
-  unsigned long min_rate,
-  unsigned long max_rate,
-  unsigned long *best_parent_rate,
-  struct clk_hw **best_parent_clk);
+int omap3_noncore_dpll_determine_rate(struct clk_hw *hw,
+ struct clk_rate_request *req);
 long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
   unsigned long *parent_rate);
 unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
@@ -273,12 +269,8 @@ unsigned long omap4_dpll_regm4xen_recalc(struct clk_hw *hw,
 long omap4_dpll_regm4xen_round_rate(struct clk_hw *hw,
unsigned long target_rate,
unsigned long *parent_rate);
-long omap4_dpll_regm4xen_determine_rate(struct clk_hw *hw,
-   unsigned long rate,
-   unsigned long min_rate,
-   unsigned long max_rate,
-   unsigned long *best_parent_rate,
-   struct clk_hw **best_parent_clk);
+int omap4_dpll_regm4xen_determine_rate(struct clk_hw *hw,
+  struct clk_rate_request *req);
 
 extern struct ti_clk_ll_ops *ti_clk_ll_ops;
 


--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] clk: ti: clock driver code migration to drivers

[Stephen Boyd]

On 07/14/2015 01:09 PM, Tero Kristo wrote:

On 07/14/2015 10:29 PM, Stephen Boyd wrote:

On 07/14/2015 01:54 AM, Tero Kristo wrote:

The following changes since commit
bc0195aad0daa2ad5b0d76cce22b167bc3435590:

  Linux 4.2-rc2 (2015-07-12 15:10:30 -0700)


Why did this get rebased onto v4.2-rc2? I thought it was all ready to go
based on v4.1-rc2? If possible leave it on v4.1-rc2 please.


You mean 4.2-rc1 I guess...? Or do you really mean the old 4.1-rc2?


Old 4.1-rc2 please

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 00/27] ARM: OMAP2+: clock code move under clk driver

[Stephen Boyd]

On 07/13/2015 02:48 AM, Tero Kristo wrote:

On 06/04/2015 02:11 AM, Michael Turquette wrote:


Hi Tero,

I'd like more time for any regressions this introduces to be fixed, so
lets push back to next merge window. The always-wrong-but-never-by-much
crystal ball[0] predicts June 14. This is less than two weeks away, so
the wait should be short.


Hi Mike / Stephen,

I am planning to send a pull-request out for this, but this set is 
going to cause some minor merge conflicts with the recent header file 
change patches from Stephen, and the determine rate prototype change 
from Boris.


I can alternatively just ignore these issues and send these patches on 
top of rc2 and let you handle the conflicts within clk-tree, or 
cherry-pick some stable commits for the same if you like.


Which way do you prefer?


It doesn't sound too bad to resolve those conflicts so send the branch 
that you already made based on v4.1-rc2.


--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] clk: change clk_ops' -determine_rate() prototype

[Stephen Boyd]

On 07/09, Boris Brezillon wrote:
 Hi Stephen,
 
 On Wed, 08 Jul 2015 11:07:42 -0700
 Stephen Boyd sb...@codeaurora.org wrote:
 
  On 07/08/2015 02:00 AM, Boris Brezillon wrote:
   Hi Stephen,
  
   On Tue, 7 Jul 2015 17:57:48 -0700
   Stephen Boyd sb...@codeaurora.org wrote:
  
   On 07/07, Boris Brezillon wrote:
  
   } else {
   pr_err(clk: clk_composite_determine_rate function 
   called, but no mux or rate callback set!\n);
   +   req-rate = 0;
   return 0;
   Shouldn't this return an error now? And then assigning req-rate
   wouldn't be necessary. Sorry I must have missed this last round.
  
   Actually I wanted to keep the existing behavior: return a 0 rate (not
   an error) when there is no mux or rate ops.
  
   That's something we can change afterwards, but it might reveals
   new bugs if some users are checking for a 0 rate to detect errors.
  
  
  Ok. Care to send the patch now to do that while we're thinking about it?
  We can test it out for a month or two.
  
 
 Here is a patch modifying a few drivers to return errors instead of a 0
 rate. Feel free to squash it in the previous one if you think this is
 better.
 
 Best Regards,
 
 Boris
 
 --- 8 ---
 
 From dca9c28301042cf19dad4b1e4555cdb7c1063745 Mon Sep 17 00:00:00 2001
 From: Boris Brezillon boris.brezil...@free-electrons.com
 Date: Thu, 9 Jul 2015 12:20:21 +0200
 Subject: [PATCH] clk: fix some determine_rate implementations
 
 Some determine_rate implementations are not returning an error when then
 failed to adapt the rate according to the rate request.
 Fix them so that they return an error instead of silently returning 0.
 
 Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com

The linewrap is seriously messed up here. Please fix your mailer
next time. I had to hand edit the patch to get it to apply. I've
applied this in top of the original patch as a different commit,
in case we need to revert it later.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] clk: change clk_ops' -determine_rate() prototype

[Stephen Boyd]

On 07/08/2015 02:00 AM, Boris Brezillon wrote:
 Hi Stephen,

 On Tue, 7 Jul 2015 17:57:48 -0700
 Stephen Boyd sb...@codeaurora.org wrote:

 On 07/07, Boris Brezillon wrote:

 } else {
 pr_err(clk: clk_composite_determine_rate function called, but 
 no mux or rate callback set!\n);
 +   req-rate = 0;
 return 0;
 Shouldn't this return an error now? And then assigning req-rate
 wouldn't be necessary. Sorry I must have missed this last round.

 Actually I wanted to keep the existing behavior: return a 0 rate (not
 an error) when there is no mux or rate ops.

 That's something we can change afterwards, but it might reveals
 new bugs if some users are checking for a 0 rate to detect errors.


Ok. Care to send the patch now to do that while we're thinking about it?
We can test it out for a month or two.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] clk: change clk_ops' -determine_rate() prototype

[Stephen Boyd]

On 07/07, Boris Brezillon wrote:
 Clock rates are stored in an unsigned long field, but -determine_rate()
 (which returns a rounded rate from a requested one) returns a long
 value (errors are reported using negative error codes), which can lead
 to long overflow if the clock rate exceed 2Ghz.
 
 Change -determine_rate() prototype to return 0 or an error code, and pass
 a pointer to a clk_rate_request structure containing the expected target
 rate and the rate constraints imposed by clk users.
 
 The clk_rate_request structure might be extended in the future to contain
 other kind of constraints like the rounding policy, the maximum clock
 inaccuracy or other things that are not yet supported by the CCF
 (power consumption constraints ?).
 
 Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
 
 CC: Jonathan Corbet cor...@lwn.net
 CC: Tony Lindgren t...@atomide.com
 CC: Ralf Baechle r...@linux-mips.org
 CC: Emilio López emi...@elopez.com.ar
 CC: Maxime Ripard maxime.rip...@free-electrons.com
 CC: Tero Kristo t-kri...@ti.com
 CC: Peter De Schrijver pdeschrij...@nvidia.com
 CC: Prashant Gaikwad pgaik...@nvidia.com
 CC: Stephen Warren swar...@wwwdotorg.org
 CC: Thierry Reding thierry.red...@gmail.com
 CC: Alexandre Courbot gnu...@gmail.com
 CC: linux-...@vger.kernel.org
 CC: linux-ker...@vger.kernel.org
 CC: linux-arm-ker...@lists.infradead.org
 CC: linux-omap@vger.kernel.org
 CC: linux-m...@linux-mips.org
 CC: linux-te...@vger.kernel.org
 
 ---

I'll throw this patch into -next now to see if any other problems
shake out. I'm hoping we get some more acks though, so it'll be
on it's own branch and become immutable in a week or so. One
question below.

 diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
 index 616f5ae..9e69f34 100644
 --- a/drivers/clk/clk-composite.c
 +++ b/drivers/clk/clk-composite.c
 @@ -99,33 +99,33 @@ static long clk_composite_determine_rate(struct clk_hw 
 *hw, unsigned long rate,
  
   parent_rate = __clk_get_rate(parent);
  
 - tmp_rate = rate_ops-round_rate(rate_hw, rate,
 + tmp_rate = rate_ops-round_rate(rate_hw, req-rate,
   parent_rate);
   if (tmp_rate  0)
   continue;
  
 - rate_diff = abs(rate - tmp_rate);
 + rate_diff = abs(req-rate - tmp_rate);
  
 - if (!rate_diff || !*best_parent_p
 + if (!rate_diff || !req-best_parent_hw
  || best_rate_diff  rate_diff) {
 - *best_parent_p = __clk_get_hw(parent);
 - *best_parent_rate = parent_rate;
 + req-best_parent_hw = __clk_get_hw(parent);
 + req-best_parent_rate = parent_rate;
   best_rate_diff = rate_diff;
   best_rate = tmp_rate;
   }
  
   if (!rate_diff)
 - return rate;
 + return 0;
   }
  
 - return best_rate;
 + req-rate = best_rate;
 + return 0;
   } else if (mux_hw  mux_ops  mux_ops-determine_rate) {
   __clk_hw_set_clk(mux_hw, hw);
 - return mux_ops-determine_rate(mux_hw, rate, min_rate,
 -max_rate, best_parent_rate,
 -best_parent_p);
 + return mux_ops-determine_rate(mux_hw, req);
   } else {
   pr_err(clk: clk_composite_determine_rate function called, but 
 no mux or rate callback set!\n);
 + req-rate = 0;
   return 0;

Shouldn't this return an error now? And then assigning req-rate
wouldn't be necessary. Sorry I must have missed this last round.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] clk: change clk_ops' -determine_rate() prototype

[Stephen Boyd]

On 07/06, Boris Brezillon wrote:
 Clock rates are stored in an unsigned long field, but -determine_rate()
 (which returns a rounded rate from a requested one) returns a long
 value (errors are reported using negative error codes), which can lead
 to long overflow if the clock rate exceed 2Ghz.
 
 Change -determine_rate() prototype to return 0 or an error code, and pass
 a pointer to a clk_rate_request structure containing the expected target
 rate and the rate constraints imposed by clk users.
 
 The clk_rate_request structure might be extended in the future to contain
 other kind of constraints like the rounding policy, the maximum clock
 inaccuracy or other things that are not yet supported by the CCF
 (power consumption constraints ?).
 
 Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com

Which files did you compile? 

drivers/clk/mmp/clk-mix.c: In function ‘mmp_clk_mix_determine_rate’:
drivers/clk/mmp/clk-mix.c:221:13: error: ‘rate’ undeclared (first use in this 
function)

 
 diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
 index 44e57ec..cd27457 100644
 --- a/arch/arm/mach-omap2/dpll3xxx.c
 +++ b/arch/arm/mach-omap2/dpll3xxx.c
 @@ -462,43 +462,38 @@ void omap3_noncore_dpll_disable(struct clk_hw *hw)
  /**
   * omap3_noncore_dpll_determine_rate - determine rate for a DPLL
   * @hw: pointer to the clock to determine rate for
 - * @rate: target rate for the DPLL
 - * @best_parent_rate: pointer for returning best parent rate
 - * @best_parent_clk: pointer for returning best parent clock
 + * @req: target rate request
   *
   * Determines which DPLL mode to use for reaching a desired target rate.
   * Checks whether the DPLL shall be in bypass or locked mode, and if
   * locked, calculates the M,N values for the DPLL via round-rate.
 - * Returns a positive clock rate with success, negative error value
 - * in failure.
 + * Returns a 0 on success, negative error value in failure.
   */
 -long omap3_noncore_dpll_determine_rate(struct clk_hw *hw, unsigned long rate,
 -unsigned long min_rate,
 -unsigned long max_rate,
 -unsigned long *best_parent_rate,
 -struct clk_hw **best_parent_clk)
 +int omap3_noncore_dpll_determine_rate(struct clk_hw *hw,
 +   struct clk_rate_request *req)
  {
   struct clk_hw_omap *clk = to_clk_hw_omap(hw);
   struct dpll_data *dd;
  
 - if (!hw || !rate)
 + if (!hw || !req || !req-rate)

Why do we need to check for req? It shouldn't be NULL.

   return -EINVAL;
  
   dd = clk-dpll_data;
   if (!dd)
   return -EINVAL;
  
 - if (__clk_get_rate(dd-clk_bypass) == rate 
 + if (__clk_get_rate(dd-clk_bypass) == req-rate 
   (dd-modes  (1  DPLL_LOW_POWER_BYPASS))) {
 - *best_parent_clk = __clk_get_hw(dd-clk_bypass);
 + req-best_parent_hw = __clk_get_hw(dd-clk_bypass);
   } else {
 - rate = omap2_dpll_round_rate(hw, rate, best_parent_rate);
 - *best_parent_clk = __clk_get_hw(dd-clk_ref);
 + req-rate = omap2_dpll_round_rate(hw, req-rate,
 +   req-best_parent_rate);
 + req-best_parent_hw = __clk_get_hw(dd-clk_ref);
   }
  
 - *best_parent_rate = rate;
 + req-best_parent_rate = req-rate;
  
 - return rate;
 + return 0;
  }
  
  /**
 diff --git a/arch/arm/mach-omap2/dpll44xx.c b/arch/arm/mach-omap2/dpll44xx.c
 index f231be0..d615571 100644
 --- a/arch/arm/mach-omap2/dpll44xx.c
 +++ b/arch/arm/mach-omap2/dpll44xx.c
 @@ -191,42 +191,36 @@ out:
  /**
   * omap4_dpll_regm4xen_determine_rate - determine rate for a DPLL
   * @hw: pointer to the clock to determine rate for
 - * @rate: target rate for the DPLL
 - * @best_parent_rate: pointer for returning best parent rate
 - * @best_parent_clk: pointer for returning best parent clock
 + * @req: target rate request
   *
   * Determines which DPLL mode to use for reaching a desired rate.
   * Checks whether the DPLL shall be in bypass or locked mode, and if
   * locked, calculates the M,N values for the DPLL via round-rate.
 - * Returns a positive clock rate with success, negative error value
 - * in failure.
 + * Returns 0 on success and a negative error value otherwise.
   */
 -long omap4_dpll_regm4xen_determine_rate(struct clk_hw *hw, unsigned long 
 rate,
 - unsigned long min_rate,
 - unsigned long max_rate,
 - unsigned long *best_parent_rate,
 - struct clk_hw **best_parent_clk)
 +int omap4_dpll_regm4xen_determine_rate(struct clk_hw *hw,
 +struct clk_rate_request *req)
  {
   struct clk_hw_omap *clk = to_clk_hw_omap(hw);
   struct dpll_data *dd;
  
 - if 

Re: [PATCH 3/5] clk: ti: Use of_clk_get_parent_count() instead of open coding

[Stephen Boyd]

On 05/29, Geert Uytterhoeven wrote:
 Signed-off-by: Geert Uytterhoeven geert+rene...@glider.be
 ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] clk: at91: Use of_clk_get_parent_count() instead of open coding

[Stephen Boyd]

On 05/29, Geert Uytterhoeven wrote:
 Signed-off-by: Geert Uytterhoeven geert+rene...@glider.be
 ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] clk: st: Use of_clk_get_parent_count() instead of open coding

[Stephen Boyd]

On 05/29, Geert Uytterhoeven wrote:
 Signed-off-by: Geert Uytterhoeven geert+rene...@glider.be
 ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/NOT FOR MERGING] HACK: add global/private timers for A9

[Stephen Boyd]

On 06/04, Felipe Balbi wrote:
 On Thu, Jun 04, 2015 at 03:18:25PM -0500, Felipe Balbi wrote:
  On Thu, Jun 04, 2015 at 03:08:50PM -0500, Felipe Balbi wrote:
   Hi,
   
   On Thu, Jun 04, 2015 at 11:46:59AM +0200, Mason wrote:
Also, check /proc/timer_list for a Broadcast device. If you don't
define one, the TWD timers are set to periodic mode, with hrtimers
disabled.
   
   Did you manage to turn global timer into Broadcast device ?
  
  arm_global_timer is marked PERCPU, so it will never be chosen as
  broadcast.
 
 Perhaps this is acceptable ?
 
 diff --git a/drivers/clocksource/arm_global_timer.c 
 b/drivers/clocksource/arm_global_timer.c
 index e6833771a716..8c0170ac367d 100644
 --- a/drivers/clocksource/arm_global_timer.c
 +++ b/drivers/clocksource/arm_global_timer.c
 @@ -169,8 +169,9 @@ static int gt_clockevents_init(struct clock_event_device 
 *clk)
   int cpu = smp_processor_id();
  
   clk-name = arm_global_timer;
 - clk-features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
 - CLOCK_EVT_FEAT_PERCPU;
 + clk-features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
 + if (is_smp() || setup_max_cpus)
 + clk-features |= CLOCK_EVT_FEAT_PERCPU;
   clk-set_mode = gt_clockevent_set_mode;
   clk-set_next_event = gt_clockevent_set_next_event;
   clk-cpumask = cpumask_of(cpu);
 

What is this doing? Allowing the global timer to become the
broadcast timer? Can you share all the clockevents that are being
registered in the system and what ratings and features they have?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] [RFC] ARM: timer-sp: Use of_clk_get_parent_count() instead of open coding

[Stephen Boyd]

On 06/04, Geert Uytterhoeven wrote:
 Hi Stephen,
 
 On Thu, Jun 4, 2015 at 1:44 AM, Stephen Boyd sb...@codeaurora.org wrote:
  On 05/29, Geert Uytterhoeven wrote:
  Signed-off-by: Geert Uytterhoeven geert+rene...@glider.be
   #include linux/err.h
   #include linux/interrupt.h
   #include linux/irq.h
  @@ -234,7 +235,7 @@ static void __init sp804_of_init(struct device_node 
  *np)
clk1 = NULL;
 
/* Get the 2nd clock if the timer has 3 timer clocks */
  - if (of_count_phandle_with_args(np, clocks, #clock-cells) == 3) {
  + if (of_clk_get_parent_count(np) == 3) {
 
  So maybe it means if we want to expose of_clk_get_parent_count()
  we should move it to some other header file (of_clk.h?). And then
  maybe we should always compile said helpers if OF=y and
  HAVE_CLK=y.
 
 That makes sense.
 
 Currently we have lots of of_clk_*() in linux/clk-provider.h, and a few
 of_clk_get*() in inux/clk.h.
 And the mysterious of_clk_set_defaults() in linux/clk/clk-conf.h.
 
 All of_clk_get*() could be moved to a new linux/of_clk.h.
 
 Alternatively, of_clk_get_parent_count() could just move to linux/clk.h?

Hmm... of_clk_*() functions that return a struct clk * or work
with a struct clk * should stay within clk.h. But these ones in
clk-provider.h should probably move to a new of_clk.h file:

of_clk_get_parent_count
of_clk_get_parent_name
of_clk_init

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/12] ARM: OMAP2+: Add support for initializing dm814x clocks

[Stephen Boyd]

On 06/03, Tony Lindgren wrote:
 diff --git a/drivers/clk/ti/clk-814x.c b/drivers/clk/ti/clk-814x.c
 new file mode 100644
 index 000..bd2353c
 --- /dev/null
 +++ b/drivers/clk/ti/clk-814x.c
 @@ -0,0 +1,36 @@
 +/*
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation version 2.
 + */
 +
 +#include linux/kernel.h
 +#include linux/list.h

Is list.h used?

 +#include linux/clk-provider.h
 +#include linux/clk/ti.h
 +
 +static struct ti_dt_clk dm814_clks[] = {
 + DT_CLK(NULL, devosc_ck, devosc_ck),
 + DT_CLK(NULL, mpu_ck, mpu_ck),
 + DT_CLK(NULL, sysclk4_ck, sysclk4_ck),
 + DT_CLK(NULL, sysclk6_ck, sysclk6_ck),
 + DT_CLK(NULL, sysclk10_ck, sysclk10_ck),
 + DT_CLK(NULL, sysclk18_ck, sysclk18_ck),
 + DT_CLK(NULL, timer_sys_ck, devosc_ck),
 + DT_CLK(NULL, cpsw_125mhz_gclk, cpsw_125mhz_gclk),
 + DT_CLK(NULL, cpsw_cpts_rft_clk, cpsw_cpts_rft_clk),
 + { .node_name = NULL },
 +};
 +
 +static const char *enable_init_clks[] = {
 +};

delete?

 +
 +int __init dm814x_dt_clk_init(void)
 +{
 + ti_dt_clocks_register(dm814_clks);
 + omap2_clk_disable_autoidle_all();
 + omap2_clk_enable_init_clocks(enable_init_clks,
 +  ARRAY_SIZE(enable_init_clks));

And pass NULL and 0 here?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/NOT FOR MERGING] HACK: add global/private timers for A9

[Stephen Boyd]

On 06/03/2015 02:28 PM, Felipe Balbi wrote:

On Wed, Jun 03, 2015 at 04:04:55PM -0500, Felipe Balbi wrote:

On Wed, Jun 03, 2015 at 10:55:27PM +0200, Arnd Bergmann wrote:

On Wednesday 03 June 2015 15:32:45 Felipe Balbi wrote:

Hi Tony and Russell,

AM43xx, even though it's a single processor A9, it still has TWD and global
timer. I was doing some profiling with RT v4.0 and latency is 3.5x lower just
by switching from gptimer to twd/global.

The only problem is that currently, is_smp() check prevents me from using twd
with AM43xx (that's why it's commented below, for testing purposes).

In the hopes that we can start a, hopefully, small thread around the subject,
I'm sending this HACK which I used to get TWD and global timer enabled so I
could measure latencies with cyclictest.

Is it so that TWD shouldn't be available on UP integrations of ARM's Cortex-A
processors ?



I wondered about this recently when looking at something unrelated
and noticed that the check had been introduced as part of
904464b91eca8 (ARM: 7655/1: smp_twd: make twd_local_timer_of_register()
no-op for nosmp).

I suspect this was just the wrong fix at the time, and that the
real culprit is either alloc_percpu() or request_percpu_irq()
getting called too early on a machine without SMP support.

Possibly the problem is already resolved independently, if you
didn't run into it.

no, no splats, nothing at all. See [1]

[1] http://hastebin.com/helekubutu

Adding Shawn



Mason was also interested in doing this. See [2]. From what I could tell 
back then, commit 904464b91eca8 was working around the local timer APIs 
that no longer exist.


[2] http://thread.gmane.org/gmane.linux.ports.arm.kernel/389931/focus=392348

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] clk: change clk_ops' -determine_rate() prototype

[Stephen Boyd]

On 05/20, Boris Brezillon wrote:
 Clock rates are stored in an unsigned long field, but -determine_rate()
 (which returns a rounded rate from a requested one) returns a long
 value (errors are reported using negative error codes), which can lead
 to long overflow if the clock rate exceed 2Ghz.
 
 Change -determine_rate() prototype to return 0 or an error code, and pass
 a pointer to a clk_rate_request structure containing the expected target
 rate and the rate constraints imposed by clk users.
 
 The clk_rate_request structure might be extended in the future to contain
 other kind of constraints like the rounding policy, the maximum clock
 inaccuracy or other things that are not yet supported by the CCF
 (power consumption constraints ?).
 
 Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
 
 CC: Jonathan Corbet cor...@lwn.net
 CC: Tony Lindgren t...@atomide.com
 CC: Ralf Baechle r...@linux-mips.org
 CC: Emilio López emi...@elopez.com.ar
 CC: Maxime Ripard maxime.rip...@free-electrons.com
 CC: Tero Kristo t-kri...@ti.com
 CC: linux-...@vger.kernel.org
 CC: linux-ker...@vger.kernel.org
 CC: linux-arm-ker...@lists.infradead.org
 CC: linux-omap@vger.kernel.org
 CC: linux-m...@linux-mips.org
 ---
 
 Hi Stephen,
 
 This patch is based on clk-next and contains the changes you suggested
 in your previous review.
 
 It was tested on sama5d4 and compile tested on several ARM platforms
 (those enabled in multi_v7_defconfig).
 

Thanks. I think we should wait until the next -rc1 drops to apply the
patch for the next merge window. That will make it least likely to conflict
with other trees, and we can provide it on a stable branch should there
be clock providers going through other trees somewhere. Please
remind me if I forget.

 @@ -1186,15 +1191,21 @@ EXPORT_SYMBOL_GPL(__clk_determine_rate);
   */
  unsigned long __clk_round_rate(struct clk *clk, unsigned long rate)
  {
 - unsigned long min_rate;
 - unsigned long max_rate;
 +
 + struct clk_rate_request req;
 + int ret;
  
   if (!clk)
   return 0;
  
 - clk_core_get_boundaries(clk-core, min_rate, max_rate);
 + clk_core_get_boundaries(clk-core, req.min_rate, req.max_rate);
 + req.rate = rate;
 +
 + ret = clk_core_round_rate_nolock(clk-core, req);
 + if (ret)
 + return ret;

This returns a negative int for unsigned long. Is that intentional?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] [RFC] ARM: timer-sp: Use of_clk_get_parent_count() instead of open coding

[Stephen Boyd]

On 05/29, Geert Uytterhoeven wrote:
 Signed-off-by: Geert Uytterhoeven geert+rene...@glider.be
 ---
 This is an RFC, as it depends on [RFC] clk: Provide dummy
 of_clk_get_parent_count() for !OF/!CCF.
 ---
  arch/arm/common/timer-sp.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
 
 diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c
 index 19211324772f387c..be87a5d480ffb655 100644
 --- a/arch/arm/common/timer-sp.c
 +++ b/arch/arm/common/timer-sp.c
 @@ -21,6 +21,7 @@
  #include linux/clk.h
  #include linux/clocksource.h
  #include linux/clockchips.h
 +#include linux/clk-provider.h

This looks weird. This is not a clock provider.

  #include linux/err.h
  #include linux/interrupt.h
  #include linux/irq.h
 @@ -234,7 +235,7 @@ static void __init sp804_of_init(struct device_node *np)
   clk1 = NULL;
  
   /* Get the 2nd clock if the timer has 3 timer clocks */
 - if (of_count_phandle_with_args(np, clocks, #clock-cells) == 3) {
 + if (of_clk_get_parent_count(np) == 3) {

So maybe it means if we want to expose of_clk_get_parent_count()
we should move it to some other header file (of_clk.h?). And then
maybe we should always compile said helpers if OF=y and
HAVE_CLK=y.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 10/10] CLK: TI: always enable DESHDCP clock

[Stephen Boyd]

On 05/28, Tero Kristo wrote:
 On 05/28/2015 07:22 AM, Michael Turquette wrote:
 Just chiming in on the critical clock discussion. I'm not planning to
 merge something that lets Devicetree nodes call clk_enable on a clock.
 That's what drivers are for.
 
 The assigned-rate and assigned-parent stuff that Tero mentioned is more
 like configuration data for a downstream clock consumer. Clock
 gating/ungating does not fall under this type of configuration data in
 my opinion.
 
 I think that Tomi's patch to call clk_prepare_enable from
 dra7xx_dt_clk_init is a reasonable solution to the problem.
 
 Yea, after this discussion I am fine with this approach also, seeing
 it apparently doesn't cause any ill side-effects.

Well hopefully when the clk is prepared and enabled it isn't
orphaned. Or we're going to be in the same problem as we're
currently in with Sunxi and trying to make EPROBE_DEFER come out
of clk_get().

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 02/27] clk: ti: move generic OMAP DPLL implementation under drivers/clk

[Stephen Boyd]

On 05/25, Tero Kristo wrote:
 @@ -281,7 +282,7 @@ unsigned long omap2_get_dpll_rate(struct clk_hw_omap *clk)
   * be rounded, or the rounded rate upon success.
   */
  long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
 - unsigned long *parent_rate)
 +unsigned long *parent_rate)
  {
   struct clk_hw_omap *clk = to_clk_hw_omap(hw);
   int m, n, r, scaled_max_m;
 @@ -310,7 +311,6 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned 
 long target_rate,
   dd-last_rounded_rate = 0;
  
   for (n = dd-min_divider; n = dd-max_divider; n++) {
 -
   /* Is the (input clk, divider) pair valid for the DPLL? */
   r = _dpll_test_fint(clk, n);
   if (r == DPLL_FINT_UNDERFLOW)
 @@ -367,4 +367,3 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned 
 long target_rate,
  
   return dd-last_rounded_rate;
  }
 -

It's a lot easier to see the cleanup that happened while copying
code over. Thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 01/27] ARM: OMAP2+: clock: export driver API to setup/get clock features

[Stephen Boyd]

On 05/25, Tero Kristo wrote:
 +
 +/**
 + * ti_clk_get_features - get clock driver features flags
 + *
 + * Get TI clock driver features description. Returns a pointer
 + * to the current feature setup.
 + */
 +struct ti_clk_features *ti_clk_get_features(void)

const?

 +{
 + return ti_clk_features;
 +}
  #endif

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 02/27] clk: ti: move generic OMAP DPLL implementation under drivers/clk

[Stephen Boyd]

On 05/20, Tero Kristo wrote:
 On 05/20/2015 04:20 AM, Stephen Boyd wrote:
 On 05/11, Tero Kristo wrote:
 With the legacy clock data now gone, we can start moving OMAP clock
 type implementations under clock driver. Start this with moving the
 generic OMAP DPLL clock type under TI clock driver.
 
 Signed-off-by: Tero Kristo t-kri...@ti.com
 ---
 
 How much of the code is the same from the original copy? Can you
 generate these patches with -M -C so that we can see?
 
 Looks like for the move patches the range is from 85%...95% exact
 copy. Shall I repost the patches in this format?

Sure, a resend would be nice to actually see what changed. If
that's too much mail, maybe you can share the diffstat for the
whole series with -M -C so we can see what's moved instead.

 Good hint for the
 -M -C btw, I wonder why those are not default options for
 git-format-patch.
 

I think it isn't the default because 'patch' the program didn't
always support the diff format that git generates with the -M and
-C options. FWIW, I have

[diff] 
renames = copy

in my .gitconfig for this purpose.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 10/10] CLK: TI: always enable DESHDCP clock

[Stephen Boyd]

On 05/20/15 04:50, Tero Kristo wrote:


 @@ -348,5 +348,10 @@ int __init dra7xx_dt_clk_init(void)
   if (rc)
   pr_err(%s: failed to set USB_DPLL M2 OUT\n, __func__);

 +hdcp_ck = clk_get_sys(NULL, dss_deshdcp_clk);
 +rc = clk_prepare_enable(hdcp_ck);
 +if (rc)
 +pr_err(%s: failed to set dss_deshdcp_clk\n, __func__);
 +
   return rc;
   }


 You should rather use the assigned-clock properties in DT to accomplish
 this, the manual clock tweaks under the drivers/clk/ti/clk-* files
 should be converted to DT setup also.

 Now that I sent this, I realize we only have support to set_parent /
 set_rate through the assigned-clock props, no enable. Any plans to
 extend this support Mike/Stephen?



Enable falls under the critical clocks discussion that is ongoing. I
assume that this is some sort of critical clock that can't be turned off?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/14] clkdev: add clkdev_create() helper

[Stephen Boyd]

On 04/07/15 05:43, Russell King - ARM Linux wrote:
 On Mon, Apr 06, 2015 at 01:19:33PM -0700, Stephen Boyd wrote:
 On 04/03/15 10:12, Russell King wrote:
 @@ -316,6 +329,29 @@ clkdev_alloc(struct clk *clk, const char *con_id, 
 const char *dev_fmt, ...)
  }
  EXPORT_SYMBOL(clkdev_alloc);
  
 +/**
 + * clkdev_create - allocate and add a clkdev lookup structure
 + * @clk: struct clk to associate with all clk_lookups
 + * @con_id: connection ID string on device
 + * @dev_fmt: format string describing device name
 + *
 + * Returns a clk_lookup structure, which can be later unregistered and
 + * freed.
 Please add that this returns NULL on failure.
 Will do, but please remember that _I'm_ taking the clkdev patches through
 my tree, as I'm the maintainer for clkdev.  Thanks.


Sounds good to me. Thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/14] clkdev: get rid of redundant clk_add_alias() prototype in linux/clk.h

[Stephen Boyd]

On 04/04/15 05:43, Robert Jarzmik wrote:
 Russell King rmk+ker...@arm.linux.org.uk writes:

 clk_add_alias() is provided by clkdev, and is not part of the clk API.
 Howver, it is prototyped in two locations: linux/clkdev.h and
 linux/clk.h.  This is a mess.  Get rid of the redundant and unnecessary
 version in linux/clk.h.

 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 Tested-by: Robert Jarzmik robert.jarz...@free.fr

 Actually, this serie fixes a regression I've seen in linux-next, and which was
 triggering the Oops in [1] on lubbock. With your serie, the kernel boots fine.



Is this with the lubbock_defconfig? Is it a regression in 4.0-rc series
or is it due to some pending -next patches interacting with the per-user
clock patches? It looks like the latter because __clk_get_hw() should be
inlined on lubbock_defconfig where CONFIG_COMMON_CLK=n.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/14] clkdev: add clkdev_create() helper

[Stephen Boyd]

On 04/03/15 10:12, Russell King wrote:
 @@ -316,6 +329,29 @@ clkdev_alloc(struct clk *clk, const char *con_id, const 
 char *dev_fmt, ...)
  }
  EXPORT_SYMBOL(clkdev_alloc);
  
 +/**
 + * clkdev_create - allocate and add a clkdev lookup structure
 + * @clk: struct clk to associate with all clk_lookups
 + * @con_id: connection ID string on device
 + * @dev_fmt: format string describing device name
 + *
 + * Returns a clk_lookup structure, which can be later unregistered and
 + * freed.

Please add that this returns NULL on failure.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] clkdev: use clk_hw internally

[Stephen Boyd]

On 04/03/15 10:12, Russell King wrote:
 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk

Some commit text would be nice for us 5 years from now when we wonder
why this patch was applied.

I suspect the commit text would go like this:

clk_add_alias() calls clk_get() followed by clk_put() but in between
those two calls it saves away the struct clk pointer to a clk_lookup
structure. This leaves the 'clk' member of the clk_lookup pointing at
freed memory on configurations where CONFIG_COMMON_CLK=y. This is a
problem because clk_get_sys() will eventually try to dereference the
freed pointer by calling __clk_get_hw() on it. Fix this by saving away
the struct clk_hw pointer instead of the struct clk pointer so that when
we try to create a per-user struct clk in clk_get_sys() we don't
dereference a junk pointer.

Now the question is does any of this matter for the 4.0 release. From
what I can tell, the answer is no.

$ git grep 'clk_add_alias' v4.0-rc6
v4.0-rc6:arch/arm/mach-davinci/da850.c: clk_add_alias(async, 
da850_cpufreq_device.name,
v4.0-rc6:arch/arm/mach-omap1/board-nokia770.c:  clk_add_alias(hwa_sys_ck, 
NULL, bclk, NULL);
v4.0-rc6:arch/arm/mach-pxa/eseries.c:   clk_add_alias(CLK_CK48M, 
e740_t7l66xb_device.name,
v4.0-rc6:arch/arm/mach-pxa/eseries.c:   clk_add_alias(CLK_CK3P6MI, 
e750_tc6393xb_device.name,
v4.0-rc6:arch/arm/mach-pxa/eseries.c:   clk_add_alias(CLK_CK3P6MI, 
e800_tc6393xb_device.name,
v4.0-rc6:arch/arm/mach-pxa/lubbock.c:   clk_add_alias(SA_CLK, NULL, 
GPIO11_CLK, NULL);
v4.0-rc6:arch/arm/mach-pxa/tosa.c:  clk_add_alias(CLK_CK3P6MI, 
tc6393xb_device.name, GPIO11_CLK, NULL);
v4.0-rc6:arch/mips/alchemy/common/clock.c:  
clk_add_alias(t-alias, NULL, t-base, NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(wdt, NULL, ahb, NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(uart, NULL, ahb, 
NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(wdt, NULL, ahb, NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(uart, NULL, ahb, 
NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(wdt, NULL, ahb, NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(uart, NULL, ahb, 
NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(wdt, NULL, ahb, NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(uart, NULL, ref, 
NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(wdt, NULL, ref, NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(uart, NULL, ref, 
NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(wdt, NULL, ref, NULL);
v4.0-rc6:arch/mips/ath79/clock.c:   clk_add_alias(uart, NULL, ref, 
NULL);
v4.0-rc6:arch/sh/kernel/cpu/clock-cpg.c:clk_add_alias(fck, 
sh-tmu-sh3.0, peripheral_clk, NULL);
v4.0-rc6:arch/sh/kernel/cpu/clock-cpg.c:clk_add_alias(fck, 
sh-tmu.0, peripheral_clk, NULL);
v4.0-rc6:arch/sh/kernel/cpu/clock-cpg.c:clk_add_alias(fck, 
sh-tmu.1, peripheral_clk, NULL);
v4.0-rc6:arch/sh/kernel/cpu/clock-cpg.c:clk_add_alias(fck, 
sh-tmu.2, peripheral_clk, NULL);
v4.0-rc6:arch/sh/kernel/cpu/clock-cpg.c:clk_add_alias(fck, sh-mtu2, 
peripheral_clk, NULL);
v4.0-rc6:arch/sh/kernel/cpu/clock-cpg.c:clk_add_alias(fck, 
sh-cmt-16.0, peripheral_clk, NULL);
v4.0-rc6:arch/sh/kernel/cpu/clock-cpg.c:clk_add_alias(fck, 
sh-cmt-32.0, peripheral_clk, NULL);
v4.0-rc6:arch/sh/kernel/cpu/clock-cpg.c:clk_add_alias(sci_ick, NULL, 
peripheral_clk, NULL);

All of these architectures and platforms have CONFIG_COMMON_CLK=n, so
there doesn't seem to be any regression that these patches are fixing.
That isn't to say the patches are bad, just that they aren't urgent for
the upcoming release.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 linux-next] clk: constify of_device_id array

[Stephen Boyd]

On 03/31/15 11:50, Fabian Frederick wrote:
 of_device_id is always used as const.
 (See driver.of_match_table and open firmware functions)

 __initdata updated to __initconst for
 static const struct of_device_id ti_clkdm_match_table[]

 Signed-off-by: Fabian Frederick f...@skynet.be


Thanks. Applied to clk-next.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 32/35 linux-next] clk: constify of_device_id array

[Stephen Boyd]

On 03/22, Fabian Frederick wrote:
 
 
  On 18 March 2015 at 15:15 Michael Turquette mturque...@linaro.org wrote:
 
 
  Quoting Fabian Frederick (2015-03-16 12:59:06)
   of_device_id is always used as const.
   (See driver.of_match_table and open firmware functions)
  
   Signed-off-by: Fabian Frederick f...@skynet.be
 
  Acked-by: Michael Turquette mturque...@linaro.org
 
 Thanks :)
 
 btw, something I forgot to mention in changelog is the __initdata - 
 __initconst
 for ti_clkdm_match_table[]
 
 I can send it again with a new changelog if necessary ...
 

Did you want us to take this through clk-next? If so please
resend with the new commit text.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 03/12/15 20:29, Shawn Guo wrote:
 On Thu, Mar 12, 2015 at 12:43:40PM -0700, Stephen Boyd wrote:
 On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
 On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
 diff = 
 --- arch/arm/mach-imx/mach-imx6q.c
 +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
 @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
 * (external OSC), and we need to clear the bit.
 */
 -  clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
   IMX6Q_GPR1_ENET_CLK_SEL_PAD;
gpr = syscon_regmap_lookup_by_compatible(fsl,imx6q-iomuxc-gpr);
if (!IS_ERR(gpr))
 Any idea how to do the comparison here? Or should we rely that the 
 bootloader
 sets this properly (it managed already to select a frequency)? The phy has 
 no
 clock node in current DT's so we can check this.

 This has been fixed by adding a clk_is_match() helper and using that to
 compare instead of comparing raw pointers. It would be nice if we could
 replace the patch with something else that doesn't require this helper
 though. It looks like this is static board configuration, so I wonder
 why we didn't just have a DT property that indicates how the gpr should
 be configured for this particular board.
 We did not add a DT property for it, because there was already enough
 info (clock configuration) in DT for kernel to figure it out.

Ok well then if everything is in DT we don't need to be comparing clock
pointers at all, right? We should be able to write up some DT parsing
logic to figure it out?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
 On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
 diff = 
 --- arch/arm/mach-imx/mach-imx6q.c
 +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
 @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
   * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
   * (external OSC), and we need to clear the bit.
   */
 -clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
 IMX6Q_GPR1_ENET_CLK_SEL_PAD;
  gpr = syscon_regmap_lookup_by_compatible(fsl,imx6q-iomuxc-gpr);
  if (!IS_ERR(gpr))
 Any idea how to do the comparison here? Or should we rely that the bootloader
 sets this properly (it managed already to select a frequency)? The phy has no
 clock node in current DT's so we can check this.


This has been fixed by adding a clk_is_match() helper and using that to
compare instead of comparing raw pointers. It would be nice if we could
replace the patch with something else that doesn't require this helper
though. It looks like this is static board configuration, so I wonder
why we didn't just have a DT property that indicates how the gpr should
be configured for this particular board.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] clkdev: add clkdev_create() helper

[Stephen Boyd]

On 03/02/15 13:01, Russell King - ARM Linux wrote:
 On Mon, Mar 02, 2015 at 11:07:58AM -0800, Stephen Boyd wrote:
 On 03/02/15 09:06, Russell King wrote:
 Add a helper to allocate and add a clk_lookup structure.  This can not
 only be used in several places in clkdev.c to simplify the code, but
 more importantly, can be used by callers of the clkdev code to simplify
 their clkdev creation and registration.

 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 ---
  drivers/clk/clkdev.c   | 52 
 ++
  include/linux/clkdev.h |  3 +++
  2 files changed, 43 insertions(+), 12 deletions(-)

 diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
 index 043fd3633373..611b9acbad78 100644
 --- a/drivers/clk/clkdev.c
 +++ b/drivers/clk/clkdev.c
 @@ -294,6 +294,19 @@ vclkdev_alloc(struct clk *clk, const char *con_id, 
 const char *dev_fmt,
 return cla-cl;
  }
  
 +static struct clk_lookup *
 +vclkdev_create((struct clk *clk, const char *con_id, const char *dev_fmt,
 +   va_list ap)
 +{
 +   struct clk_lookup *cl;
 +
 +   cl = vclkdev_alloc(clk, con_id, dev_fmt, ap);
 +   if (cl)
 +   clkdev_add(cl);
 +
 +   return cl;
 +}
 +
  struct clk_lookup * __init_refok
  clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
  {
 @@ -308,6 +321,28 @@ clkdev_alloc(struct clk *clk, const char *con_id, 
 const char *dev_fmt, ...)
  }
  EXPORT_SYMBOL(clkdev_alloc);
  
 +/**
 + * clkdev_create - allocate and add a clkdev lookup structure
 + * @clk: struct clk to associate with all clk_lookups
 + * @con_id: connection ID string on device
 + * @dev_fmt: format string describing device name
 + *
 + * Returns a clk_lookup structure, which can be later unregistered and
 + * freed.
 And returns NULL on failure? Any reason why we don't return an error
 pointer on failure?
 Why should it when it's only error is no memory ?  It follows the
 clkdev_alloc() and memory allocator pattern.

 It'd also make the error handling in places like clk_add_alias() more
 difficult (how that happened, I don't know...) though that could probably
 be fixed as no one seems to bother checking the return value... maybe
 that's a reason to make it return void ;)


Ok, fair enough. Right now clk_add_alias() leaks if a driver decides to
add an alias and then fail probe for something like probe deferral. We
should probably make that return the clk_lookup structure too so that
drivers can clean up.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/10] initial clkdev cleanups

[Stephen Boyd]

On 03/02/15 09:05, Russell King - ARM Linux wrote:
 Here's some initial clkdev cleanups.  These are targetted for the next
 merge window, and while the initial patches can be merged independently,
 I'd prefer to keep the series together as further work on solving the
 problems which unique struct clk's has introduced is needed.

 The initial cleanups are more about using the correct clkdev function
 than anything else: there's no point interating over a array of
 clk_lookup structs, adding each one in turn when we have had a function
 which does this since forever.

The clkdev_add_table() conversions look good.


 I'm also killing a chunk of seemingly unused code in the omap3isp driver.

 Lastly, I'm introducing a clkdev_create() helper, which combines the
 clkdev_alloc() + clkdev_add() pattern which keeps cropping up.


We already have a solution to that problem with clk_register_clkdev().
Andy has done some work to make clk_register_clkdev() return a struct
clk_lookup pointer[1]. Maybe we can do that instead of introducing a new
clkdev_create() function. There is some benefit to having a new function
though so that we can avoid a flag day, although it looks like the flag
day is small in this case so it might not actually matter.

[1] https://www.marc.info/?l=linux-kernelm=142469226512289

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] clkdev: add clkdev_create() helper

[Stephen Boyd]

On 03/02/15 09:06, Russell King wrote:
 Add a helper to allocate and add a clk_lookup structure.  This can not
 only be used in several places in clkdev.c to simplify the code, but
 more importantly, can be used by callers of the clkdev code to simplify
 their clkdev creation and registration.

 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 ---
  drivers/clk/clkdev.c   | 52 
 ++
  include/linux/clkdev.h |  3 +++
  2 files changed, 43 insertions(+), 12 deletions(-)

 diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
 index 043fd3633373..611b9acbad78 100644
 --- a/drivers/clk/clkdev.c
 +++ b/drivers/clk/clkdev.c
 @@ -294,6 +294,19 @@ vclkdev_alloc(struct clk *clk, const char *con_id, const 
 char *dev_fmt,
   return cla-cl;
  }
  
 +static struct clk_lookup *
 +vclkdev_create((struct clk *clk, const char *con_id, const char *dev_fmt,
 + va_list ap)
 +{
 + struct clk_lookup *cl;
 +
 + cl = vclkdev_alloc(clk, con_id, dev_fmt, ap);
 + if (cl)
 + clkdev_add(cl);
 +
 + return cl;
 +}
 +
  struct clk_lookup * __init_refok
  clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
  {
 @@ -308,6 +321,28 @@ clkdev_alloc(struct clk *clk, const char *con_id, const 
 char *dev_fmt, ...)
  }
  EXPORT_SYMBOL(clkdev_alloc);
  
 +/**
 + * clkdev_create - allocate and add a clkdev lookup structure
 + * @clk: struct clk to associate with all clk_lookups
 + * @con_id: connection ID string on device
 + * @dev_fmt: format string describing device name
 + *
 + * Returns a clk_lookup structure, which can be later unregistered and
 + * freed.

And returns NULL on failure? Any reason why we don't return an error
pointer on failure?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/05/15 18:15, Stephen Boyd wrote:
 On 02/05/15 07:45, Quentin Lambert wrote:
 On 05/02/2015 00:26, Stephen Boyd wrote:
 If you want me to I can enlarge the search to other directories.
 Yes please do. And if you could share the coccinelle patch that would be
 great. Thanks.

 structclk.cocci is the coccinelle patch
 structclk-arm.patch is the result I got when applying it to the
 arch/arm directory

 Is there anything else I can do to help?


 Thanks for the coccinelle patch. Thinking more about it, I don't think
 we care if the pointer is dereferenced because that would require a
 definition of struct clk and that is most likely not the case outside of
 the clock framework. Did you scan the entire kernel? I'm running it now
 but it seems to be taking a while.


I ran the script on all files that include linux/clk.h. I've also
trimmed out mips and unicore32 because they're not using the common
clock framework.

diff = 
--- arch/arm/mach-imx/mach-imx6q.c
+++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
@@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
 * (external OSC), and we need to clear the bit.
 */
-   clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
   IMX6Q_GPR1_ENET_CLK_SEL_PAD;
gpr = syscon_regmap_lookup_by_compatible(fsl,imx6q-iomuxc-gpr);
if (!IS_ERR(gpr))
diff = 
--- drivers/gpu/drm/armada/armada_510.c
+++ /tmp/cocci-output-12321-a5f298-armada_510.c
@@ -53,7 +53,6 @@ static int armada510_crtc_compute_clock(
if (IS_ERR(clk))
return PTR_ERR(clk);
 
-   if (dcrtc-clk != clk) {
ret = clk_prepare_enable(clk);
if (ret)
return ret;
drivers/gpu/drm/armada/armada_510.c:56:5-15: WARNING trying to compare or 
dereference struct clk pointers.
diff = 
--- drivers/pwm/pwm-atmel-hlcdc.c
+++ /tmp/cocci-output-12679-3c5195-pwm-atmel-hlcdc.c
@@ -91,7 +91,6 @@ static int atmel_hlcdc_pwm_config(struct
 
pwmcfg = ATMEL_HLCDC_PWMPS(pres);
 
-   if (new_clk != chip-cur_clk) {
u32 gencfg = 0;
int ret;
 
drivers/pwm/pwm-atmel-hlcdc.c:94:5-12: WARNING trying to compare or dereference 
struct clk pointers.
diff = 
--- drivers/tty/serial/samsung.c
+++ /tmp/cocci-output-12827-715e72-samsung.c
@@ -750,7 +750,6 @@ static void s3c24xx_serial_set_termios(s
 
/* check to see if we need  to change clock source */
 
-   if (ourport-baudclk != clk) {
s3c24xx_serial_setsource(port, clk_sel);
 
if (!IS_ERR(ourport-baudclk)) {
drivers/tty/serial/samsung.c:753:5-21: WARNING trying to compare or dereference 
struct clk pointers.
diff = 
--- sound/soc/fsl/fsl_esai.c
+++ /tmp/cocci-output-13020-d518c3-fsl_esai.c
@@ -269,7 +269,6 @@ static int fsl_esai_set_dai_sysclk(struc
}
 
/* Only EXTAL source can be output directly without using PSR and PM */
-   if (ratio == 1  clksrc == esai_priv-extalclk) {
/* Bypass all the dividers if not being needed */
ecr |= tx ? ESAI_ECR_ETO : ESAI_ECR_ERO;
goto out;
sound/soc/fsl/fsl_esai.c:272:19-25: WARNING trying to compare or dereference 
struct clk pointers.
diff = 
--- sound/soc/fsl/fsl_spdif.c
+++ /tmp/cocci-output-13024-7acb1d-fsl_spdif.c
@@ -1054,7 +1054,6 @@ static u32 fsl_spdif_txclk_caldiv(struct
enum spdif_txrate index, bool round)
 {
const u32 rate[] = { 32000, 44100, 48000, 96000, 192000 };
-   bool is_sysclk = clk == spdif_priv-sysclk;
u64 rate_ideal, rate_actual, sub;
u32 sysclk_dfmin, sysclk_dfmax;
u32 txclk_df, sysclk_df, arate;
@@ -1148,7 +1147,6 @@ static int fsl_spdif_probe_txclk(struct
spdif_priv-txclk_src[index], rate[index]);
dev_dbg(pdev-dev, use txclk df %d for %dHz sample rate\n,
spdif_priv-txclk_df[index], rate[index]);
-   if (spdif_priv-txclk[index] == spdif_priv-sysclk)
dev_dbg(pdev-dev, use sysclk df %d for %dHz sample rate\n,
spdif_priv-sysclk_df[index], rate[index]);
dev_dbg(pdev-dev, the best rate for %dHz sample rate is %dHz\n,
sound/soc/fsl/fsl_spdif.c:1151:5-29: WARNING trying to compare or dereference 
struct clk pointers.
sound/soc/fsl/fsl_spdif.c:1057:18-21: WARNING trying to compare or dereference 
struct clk pointers.
diff = 
--- sound/soc/kirkwood/kirkwood-i2s.c
+++ /tmp/cocci-output-13041-3200a6-kirkwood-i2s.c
@@ -579,7 +579,6 @@ static int kirkwood_i2s_dev_probe(struct
if (PTR_ERR(priv-extclk) == -EPROBE_DEFER)
return -EPROBE_DEFER;
} else {
-   if (priv-extclk == priv-clk) {
devm_clk_put(pdev-dev, priv-extclk);
priv-extclk = ERR_PTR(-EINVAL);
} else

Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/06/15 05:39, Russell King - ARM Linux wrote:
 On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote:

 From what I can tell this code is
 now broken because we made all clk getting functions (there's quite a
 few...) return unique pointers every time they're called. It seems that
 the driver wants to know if extclk and clk are the same so it can do
 something differently in kirkwood_set_rate(). Do we need some sort of
 clk_equal(struct clk *a, struct clk *b) function for drivers like this?
 Well, the clocks in question are the SoC internal clock (which is more or
 less fixed, but has a programmable divider) and an externally supplied
 clock, and the IP has a multiplexer on its input which allows us to select
 between those two sources.

 If it were possible to bind both to the same clock, it wouldn't be a
 useful configuration - nothing would be gained from doing so in terms of
 available rates.

 What the comparison is there for is to catch the case with legacy lookups
 where a clkdev lookup entry with a NULL connection ID results in matching
 any connection ID passed to clk_get().  If the patch changes this, then
 we will have a regression - and this is something which needs fixing
 _before_ we do this return unique clocks.


Ok. It seems that we might need a clk_equal() or similar API after all.
My understanding is that this driver is calling clk_get() twice with
NULL for the con_id and then extclk in attempts to get the SoC
internal clock and the externally supplied clock. If we're using legacy
lookups then both clk_get() calls may map to the same clk_lookup entry
and before Tomeu's patch that returns unique clocks the driver could
detect this case and know that there isn't an external clock. Looking at
arch/arm/mach-dove/common.c it seems that there is only one lookup per
device and it has a wildcard NULL for con_id so both clk_get() calls
here are going to find the same lookup and get a unique struct clk pointer.

Why don't we make the legacy lookup more specific and actually indicate
internal for the con_id? Then the external clock would fail to be
found, but we can detect that case and figure out that it's not due to
probe defer, but instead due to the fact that there really isn't any
mapping. It looks like the code is already prepared for this anyway.

8

From: Stephen Boyd sb...@codeaurora.org
Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup

This i2s driver is using the wildcard nature of clkdev lookups to
figure out if there's an external clock or not. It does this by
calling clk_get() twice with NULL for the con_id first and then
external for the con_id the second time around and then
compares the two pointers. With DT the wildcard feature of
clk_get() is gone and so the driver has to handle an error from
the second clk_get() call as meaning no external clock
specified. Let's use that logic even with clk lookups to
simplify the code and remove the struct clk pointer comparisons
which may not work in the future when clk_get() returns unique
pointers.

Signed-off-by: Stephen Boyd sb...@codeaurora.org
---
 arch/arm/mach-dove/common.c   |  4 ++--
 sound/soc/kirkwood/kirkwood-i2s.c | 13 -
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index 0d1a89298ece..f290fc944cc1 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -124,8 +124,8 @@ static void __init dove_clk_init(void)
orion_clkdev_add(NULL, sdhci-dove.1, sdio1);
orion_clkdev_add(NULL, orion_nand, nand);
orion_clkdev_add(NULL, cafe1000-ccic.0, camera);
-   orion_clkdev_add(NULL, mvebu-audio.0, i2s0);
-   orion_clkdev_add(NULL, mvebu-audio.1, i2s1);
+   orion_clkdev_add(internal, mvebu-audio.0, i2s0);
+   orion_clkdev_add(internal, mvebu-audio.1, i2s1);
orion_clkdev_add(NULL, mv_crypto, crypto);
orion_clkdev_add(NULL, dove-ac97, ac97);
orion_clkdev_add(NULL, dove-pdma, pdma);
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c 
b/sound/soc/kirkwood/kirkwood-i2s.c
index def7d8260c4e..0bfeb712a997 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device 
*pdev)
return -EINVAL;
}
 
-   priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL);
+   priv-clk = devm_clk_get(pdev-dev, internal);
if (IS_ERR(priv-clk)) {
dev_err(pdev-dev, no clock\n);
return PTR_ERR(priv-clk);
@@ -579,14 +579,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device 
*pdev)
if (PTR_ERR(priv-extclk) == -EPROBE_DEFER)
return -EPROBE_DEFER;
} else {
-   if (priv-extclk == priv-clk) {
-   devm_clk_put(pdev-dev, priv-extclk);
-   priv-extclk = ERR_PTR(-EINVAL

Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/06/15 11:37, Russell King - ARM Linux wrote:
 On Fri, Feb 06, 2015 at 11:30:18AM -0800, Stephen Boyd wrote:
 Why don't we make the legacy lookup more specific and actually indicate
 internal for the con_id? Then the external clock would fail to be
 found, but we can detect that case and figure out that it's not due to
 probe defer, but instead due to the fact that there really isn't any
 mapping. It looks like the code is already prepared for this anyway.
 We _could_, and that would solve this specific issue, but I'd suggest
 coccinelle is used to locate any other similar instances of this.

 As I'm allergic to coccinelle (or coccinelle is allergic to me since
 I never seem to be able to get it to do what I want...)


Great. I'd like to avoid adding clk_equal() until we actually need it,
and I hope we don't ever need it. We've already got coccinelle in the
works to find similar issues and it seems you and I have the same
allergies because I can't get it to work for me right now.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/06/15 01:12, Julia Lawall wrote:

 On Fri, 6 Feb 2015, Quentin Lambert wrote:

 On 06/02/2015 03:15, Stephen Boyd wrote:
 Thanks for the coccinelle patch. Thinking more about it, I don't think
 we care if the pointer is dereferenced because that would require a
 definition of struct clk and that is most likely not the case outside of
 the clock framework. Did you scan the entire kernel?
 No I haven't.
 I'm running it now
 but it seems to be taking a while.

 Yes, that's why, as a first step, I chose to limit the scan to the arm
 directory.
 Are you sure to be using all of the options provided:

 // Options: --recursive-includes --relax-include-path
 // Options: --include-headers-for-types

 And are you using 1.0.0-rc23 or 1.0.0-rc24?  Those should save parsed
 header files so that they don't have to be parsed over and over.

 If you are using rc24, then you can use the -j option for parallelism.
 But then you should also use an option like --chunksize 10 (I don't know
 what number would be good), because the default is chunksize 1, and in
 that case the saved parsed header files are not reused, because the fies
 are all processed individually.  In general, it is only the files within a
 chunk that will share parsed header files.

Thanks for the info.

$ spatch --version
spatch version 1.0.0-rc22 with Python support and with PCRE support

so I guess I need to update. I tried throwing it into
scripts/coccinelle/misc and then did

make O=../obj/ coccicheck
COCCI=../kernel/scripts/coccinelle/misc/structclk.cocci

at the toplevel but it didn't find anything.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/05/15 12:07, Stephen Boyd wrote:
 On 02/05/15 11:44, Sylwester Nawrocki wrote:
 Hi Tomeu,

 On 23/01/15 12:03, Tomeu Vizoso wrote:
  int __clk_get(struct clk *clk)
  {
 -   if (clk) {
 -   if (!try_module_get(clk-owner))
 +   struct clk_core *core = !clk ? NULL : clk-core;
 +
 +   if (core) {
 +   if (!try_module_get(core-owner))
 return 0;
  
 -   kref_get(clk-ref);
 +   kref_get(core-ref);
 }
 return 1;
  }
  
 -void __clk_put(struct clk *clk)
 +static void clk_core_put(struct clk_core *core)
  {
 struct module *owner;
  
 -   if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
 -   return;
 +   owner = core-owner;
  
 clk_prepare_lock();
 -   owner = clk-owner;
 -   kref_put(clk-ref, __clk_release);
 +   kref_put(core-ref, __clk_release);
 clk_prepare_unlock();
  
 module_put(owner);
  }
  
 +void __clk_put(struct clk *clk)
 +{
 +   if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
 +   return;
 +
 +   clk_core_put(clk-core);
 +   kfree(clk);
 Why do we have kfree() here? clk_get() doesn't allocate the data structure 
 being freed here. What happens if we do clk_get(), clk_put(), clk_get() 
 on same clock?

 I suspect __clk_free_clk() should be called in __clk_release() callback
 instead, but then there is an issue of safely getting reference to
 struct clk from struct clk_core pointer.

 I tested linux-next on Odroid U3 and booting fails with oopses as below.
 There is no problems when the above kfree() is commented out.


 Ah now I get it. You meant to say that of_clk_get_by_clkspec() doesn't
 return an allocated clk pointer. Let's fix that.

 8

 From: Stephen Boyd sb...@codeaurora.org
 Subject: [PATCH] clkdev: Always allocate a struct clk in OF functions

 of_clk_get_by_clkspec() returns a struct clk pointer but it
 doesn't create a new handle for the consumers. Instead it just
 returns whatever the OF clk provider hands out. Let's create a
 per-user handle here so that clk_put() can properly unlink it and
 free it when the consumer is done.

 Fixes: 035a61c314eb clk: Make clk API return per-user struct clk instances
 Reported-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Stephen Boyd sb...@codeaurora.org
 ---
  drivers/clk/clkdev.c | 44 +++-
  1 file changed, 23 insertions(+), 21 deletions(-)

 diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
 index 29a1ab7af4b8..00d747d09b2a 100644
 --- a/drivers/clk/clkdev.c
 +++ b/drivers/clk/clkdev.c
 @@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex);
  
  #if defined(CONFIG_OF)  defined(CONFIG_COMMON_CLK)
  
 -/**
 - * of_clk_get_by_clkspec() - Lookup a clock form a clock provider
 - * @clkspec: pointer to a clock specifier data structure
 - *
 - * This function looks up a struct clk from the registered list of clock
 - * providers, an input is a clock specifier data structure as returned
 - * from the of_parse_phandle_with_args() function call.
 - */
 -struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
 +static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec,
 +  const char *dev_id, const char *con_id)
  {
   struct clk *clk;
  
 @@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args 
 *clkspec)
   of_clk_lock();
   clk = __of_clk_get_from_provider(clkspec);
  
 + if (!IS_ERR(clk))
 + clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id);
   if (!IS_ERR(clk)  !__clk_get(clk))
   clk = ERR_PTR(-ENOENT);


Actually we can bury the __clk_create_clk() inside
__of_clk_get_from_provider(). We should also move __clk_get() into there
because right now we have a hole where whoever calls
of_clk_get_from_provider() never calls __clk_get() on the clk, leading
to possible badness. v2 coming soon.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/05/15 16:42, Russell King - ARM Linux wrote:
 On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote:
 Actually we can bury the __clk_create_clk() inside
 __of_clk_get_from_provider(). We should also move __clk_get() into there
 because right now we have a hole where whoever calls
 of_clk_get_from_provider() never calls __clk_get() on the clk, leading
 to possible badness. v2 coming soon.
 There's some other issues here too...

 sound/soc/kirkwood/kirkwood-i2s.c:

 priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL);
 ...
 priv-extclk = devm_clk_get(pdev-dev, extclk);
 if (IS_ERR(priv-extclk)) {
 ...
   } else {
 if (priv-extclk == priv-clk) {
 devm_clk_put(pdev-dev, priv-extclk);
 priv-extclk = ERR_PTR(-EINVAL);
 } else {
 dev_info(pdev-dev, found external clock\n);
 clk_prepare_enable(priv-extclk);
 soc_dai = kirkwood_i2s_dai_extclk;
 }

 It should be fine provided your trick is only done for DT clocks,
 but not for legacy - with legacy, a NULL in the clkdev tables will
 match both these requests, hence the need to compare the clk_get()
 return value to tell whether we get the same clock.


Are we still talking about of_clk_get_from_provider()? Or are we talking
about comparing struct clk pointers? From what I can tell this code is
now broken because we made all clk getting functions (there's quite a
few...) return unique pointers every time they're called. It seems that
the driver wants to know if extclk and clk are the same so it can do
something differently in kirkwood_set_rate(). Do we need some sort of
clk_equal(struct clk *a, struct clk *b) function for drivers like this?

Also, even on DT this could fail if the DT author made internal and
extclk map to the same clock provider and cell.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/05/15 08:02, Quentin Lambert wrote:
 Sorry let me do that properly.

 On 05/02/2015 16:45, Quentin Lambert wrote:

 On 05/02/2015 00:26, Stephen Boyd wrote:
 If you want me to I can enlarge the search to other directories.
 Yes please do. And if you could share the coccinelle patch that
 would be
 great. Thanks.

 structclk.cocci is the coccinelle patch
 structclk-arm.patch is the result I got when applying it to the
 arch/arm directory

 Is there anything else I can do to help?



 These are the new instances I found by applying the patch to arch/arm
 directory:

 ./arch/arm/mach-imx/mach-imx6q.c
 @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
   * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
   * (external OSC), and we need to clear the bit.
   */
  clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
 IMX6Q_GPR1_ENET_CLK_SEL_PAD;
  gpr = syscon_regmap_lookup_by_compatible(fsl,imx6q-iomuxc-gpr);
  if (!IS_ERR(gpr))

This one looks like a real problem and it could probably use a
clk_equal(struct clk *a, struct clk *b) function.


 ./arch/arm/mach-shmobile/clock-r8a73a4.c
 @@ -139,7 +139,6 @@ static int pll_set_parent(struct clk *cl

  /* Search the parent */
  for (i = 0; i  clk-parent_num; i++)
  if (clk-parent_table[i] == parent)
  break;

  if (i == clk-parent_num)

 ./arch/arm/mach-shmobile/clock-sh7372.c
 @@ -223,7 +223,6 @@ static int pllc2_set_parent(struct clk *

  /* Search the parent */
  for (i = 0; i  clk-parent_num; i++)
  if (clk-parent_table[i] == parent)
  break;

  if (i == clk-parent_num)

 ./arch/arm/mach-shmobile/clock-r8a7740.c
 @@ -195,7 +195,6 @@ static int usb24s_set_parent(struct clk

  /* Search the parent */
  for (i = 0; i  clk-parent_num; i++)
  if (clk-parent_table[i] == parent)
  break;

  if (i == clk-parent_num)




I don't think shmobile uses the CCF so this should be safe, but we
should probably fix them up to also use a clk_equal() function that just
does pointer comparisons.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/05/15 07:45, Quentin Lambert wrote:

 On 05/02/2015 00:26, Stephen Boyd wrote:
 If you want me to I can enlarge the search to other directories.
 Yes please do. And if you could share the coccinelle patch that would be
 great. Thanks.

 structclk.cocci is the coccinelle patch
 structclk-arm.patch is the result I got when applying it to the
 arch/arm directory

 Is there anything else I can do to help?



Thanks for the coccinelle patch. Thinking more about it, I don't think
we care if the pointer is dereferenced because that would require a
definition of struct clk and that is most likely not the case outside of
the clock framework. Did you scan the entire kernel? I'm running it now
but it seems to be taking a while.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/05/15 11:44, Sylwester Nawrocki wrote:
 Hi Tomeu,

 On 23/01/15 12:03, Tomeu Vizoso wrote:
  int __clk_get(struct clk *clk)
  {
 -if (clk) {
 -if (!try_module_get(clk-owner))
 +struct clk_core *core = !clk ? NULL : clk-core;
 +
 +if (core) {
 +if (!try_module_get(core-owner))
  return 0;
  
 -kref_get(clk-ref);
 +kref_get(core-ref);
  }
  return 1;
  }
  
 -void __clk_put(struct clk *clk)
 +static void clk_core_put(struct clk_core *core)
  {
  struct module *owner;
  
 -if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
 -return;
 +owner = core-owner;
  
  clk_prepare_lock();
 -owner = clk-owner;
 -kref_put(clk-ref, __clk_release);
 +kref_put(core-ref, __clk_release);
  clk_prepare_unlock();
  
  module_put(owner);
  }
  
 +void __clk_put(struct clk *clk)
 +{
 +if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
 +return;
 +
 +clk_core_put(clk-core);
 +kfree(clk);
 Why do we have kfree() here? clk_get() doesn't allocate the data structure 
 being freed here. What happens if we do clk_get(), clk_put(), clk_get() 
 on same clock?

 I suspect __clk_free_clk() should be called in __clk_release() callback
 instead, but then there is an issue of safely getting reference to
 struct clk from struct clk_core pointer.

 I tested linux-next on Odroid U3 and booting fails with oopses as below.
 There is no problems when the above kfree() is commented out.



Ah now I get it. You meant to say that of_clk_get_by_clkspec() doesn't
return an allocated clk pointer. Let's fix that.

8

From: Stephen Boyd sb...@codeaurora.org
Subject: [PATCH] clkdev: Always allocate a struct clk in OF functions

of_clk_get_by_clkspec() returns a struct clk pointer but it
doesn't create a new handle for the consumers. Instead it just
returns whatever the OF clk provider hands out. Let's create a
per-user handle here so that clk_put() can properly unlink it and
free it when the consumer is done.

Fixes: 035a61c314eb clk: Make clk API return per-user struct clk instances
Reported-by: Sylwester Nawrocki s.nawro...@samsung.com
Signed-off-by: Stephen Boyd sb...@codeaurora.org
---
 drivers/clk/clkdev.c | 44 +++-
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 29a1ab7af4b8..00d747d09b2a 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex);
 
 #if defined(CONFIG_OF)  defined(CONFIG_COMMON_CLK)
 
-/**
- * of_clk_get_by_clkspec() - Lookup a clock form a clock provider
- * @clkspec: pointer to a clock specifier data structure
- *
- * This function looks up a struct clk from the registered list of clock
- * providers, an input is a clock specifier data structure as returned
- * from the of_parse_phandle_with_args() function call.
- */
-struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
+static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec,
+const char *dev_id, const char *con_id)
 {
struct clk *clk;
 
@@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args 
*clkspec)
of_clk_lock();
clk = __of_clk_get_from_provider(clkspec);
 
+   if (!IS_ERR(clk))
+   clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id);
if (!IS_ERR(clk)  !__clk_get(clk))
clk = ERR_PTR(-ENOENT);
 
@@ -54,7 +49,21 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args 
*clkspec)
return clk;
 }
 
-static struct clk *__of_clk_get(struct device_node *np, int index)
+/**
+ * of_clk_get_by_clkspec() - Lookup a clock form a clock provider
+ * @clkspec: pointer to a clock specifier data structure
+ *
+ * This function looks up a struct clk from the registered list of clock
+ * providers, an input is a clock specifier data structure as returned
+ * from the of_parse_phandle_with_args() function call.
+ */
+struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
+{
+   return __of_clk_get_by_clkspec(clkspec, NULL, __func__);
+}
+
+static struct clk *__of_clk_get(struct device_node *np, int index,
+  const char *dev_id, const char *con_id)
 {
struct of_phandle_args clkspec;
struct clk *clk;
@@ -68,7 +77,7 @@ static struct clk *__of_clk_get(struct device_node *np, int 
index)
if (rc)
return ERR_PTR(rc);
 
-   clk = of_clk_get_by_clkspec(clkspec);
+   clk = __of_clk_get_by_clkspec(clkspec, dev_id, con_id);
of_node_put(clkspec.np);
 
return clk;
@@ -76,12 +85,7 @@ static struct clk *__of_clk_get(struct device_node *np, int 
index)
 
 struct clk *of_clk_get(struct device_node *np, int index)
 {
-   struct clk *clk = __of_clk_get(np, index);
-
-   if (!IS_ERR(clk))
-   clk = __clk_create_clk

Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/03/15 08:04, Quentin Lambert wrote:
 Hello,
 Julia asked me to have a look and see if I can help.

 I have found these three cases using Coccinnelle in the mach-omap2
 directory.



 ./arch/arm/mach-omap2/clkt_clksel.c
 @@ -67,7 +67,6 @@ static const struct clksel *_get_clksel_
 return NULL;

 for (clks = clk-clksel; clks-parent; clks++)
 if (clks-parent == src_clk)

This probably needs to compare hw pointers again given that it's in the
clk-provider code. It would be nice if we could use indices instead though.

 break; /* Found the requested parent */

 if (!clks-parent) {
 ./arch/arm/mach-omap2/dpll3xxx.c
 @@ -551,7 +549,6 @@ int omap3_noncore_dpll_set_rate(struct c
 if (!dd)
 return -EINVAL;

 if (__clk_get_parent(hw-clk) != dd-clk_ref)

This one is what this thread has started on about. Comparing hw pointers
is ok for now...

 return -EINVAL;

 if (dd-last_rounded_rate == 0)
 ./arch/arm/mach-omap2/timer.c
 @@ -298,7 +298,6 @@ static int __init omap_dm_timer_init_one
 if (IS_ERR(src))
 return PTR_ERR(src);

 if (clk_get_parent(timer-fclk) != src) {

This one looks like an optimization. We can just always try to set the
parent and if it's already the current parent then the core should bail
out early without an error. So the fix would be to just remove the if
condition.

 r = clk_set_parent(timer-fclk, src);
 if (r  0) {
 pr_warn(%s: %s cannot set source\n, __func__,


 Are they instances of your issue?

Yes these all look like instances of the problem.

 If you want me to I can enlarge the search to other directories.

Yes please do. And if you could share the coccinelle patch that would be
great. Thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/01/15 13:24, Mike Turquette wrote:
 Quoting Tomeu Vizoso (2015-01-23 03:03:30)
 Moves clock state to struct clk_core, but takes care to change as little API 
 as
 possible.

 struct clk_hw still has a pointer to a struct clk, which is the
 implementation's per-user clk instance, for backwards compatibility.

 The struct clk that clk_get_parent() returns isn't owned by the caller, but 
 by
 the clock implementation, so the former shouldn't call clk_put() on it.

 Because some boards in mach-omap2 still register clocks statically, their 
 clock
 registration had to be updated to take into account that the clock 
 information
 is stored in struct clk_core now.
 Tero, Paul  Tony,

 Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
 struct dpll_data, namely this snippet from
 arch/arm/mach-omap2/dpll3xxx.c:

 parent = __clk_get_parent(hw-clk);

 if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
 WARN(parent != dd-clk_bypass,
 here0, parent name is %s, bypass name is 
 %s\n,
 __clk_get_name(parent), 
 __clk_get_name(dd-clk_bypass));
 r = _omap3_noncore_dpll_bypass(clk);
 } else {
 WARN(parent != dd-clk_ref,
 here1, parent name is %s, ref name is %s\n,
 __clk_get_name(parent), 
 __clk_get_name(dd-clk_ref));
 r = _omap3_noncore_dpll_lock(clk);
 }

 struct dpll_data has members clk_ref and clk_bypass which are struct clk
 pointers. This was always a bit of a violation of the clk.h contract
 since drivers are not supposed to deref struct clk pointers. 

Julia,

Is there a way we can write a coccinelle script to check for this? The
goal being to find all drivers that are comparing struct clk pointers or
attempting to dereference them. There are probably other frameworks that
could use the same type of check (regulator, gpiod, reset, pwm, etc.).
Probably anything that has a get/put API.

-Stephen

 Now that we
 generate unique pointers for each call to clk_get (clk_ref  clk_bypass
 are populated by of_clk_get in ti_clk_register_dpll) then the pointer
 comparisons above will never be equal (even if they resolve down to the
 same struct clk_core). I added the verbose traces to the WARNs above to
 illustrate the point: the names are always the same but the pointers
 differ.

 AFAICT this doesn't break anything, but booting on OMAP3+ results in
 noisy WARNs.

 I think the correct fix is to replace clk_bypass and clk_ref pointers
 with a simple integer parent_index. In fact we already have this index.
 See how the pointers are populated in ti_clk_register_dpll:


 dd-clk_ref = of_clk_get(node, 0);
 dd-clk_bypass = of_clk_get(node, 1);

 Tony, the same problem affects the FAPLL code which copy/pastes some of
 the DPLL code.

 Thoughts?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/02/15 14:41, Mike Turquette wrote:
 Quoting Tero Kristo (2015-02-02 11:32:01)
 On 02/01/2015 11:24 PM, Mike Turquette wrote:

 AFAICT this doesn't break anything, but booting on OMAP3+ results in
 noisy WARNs.

 I think the correct fix is to replace clk_bypass and clk_ref pointers
 with a simple integer parent_index. In fact we already have this index.
 See how the pointers are populated in ti_clk_register_dpll:
 The problem is we still need to be able to get runtime parent clock 
 rates (the parent rate may change also), so simple index value is not 
 sufficient. We need a handle of some sort to the bypass/ref clocks. The 
 DPLL code generally requires knowledge of the bypass + reference clock 
 rates to work properly, as it calculates the M/N values based on these.
 We can maybe introduce something like of_clk_get_parent_rate, as we have
 analogous stuff for getting parent names and indexes. Without
 introducing a new helper you could probably just do:

   clk_ref = clk_get_parent_by_index(dpll_clk, 0);
   ref_rate = clk_get_rate(clk_ref);

   clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
   bypass_rate = clk_get_rate(clk_bypass);

 Currently the semantics around this call are weird. It seems like it
 would create a new struct clk pointer but it does not. So don't call
 clk_put on clk_ref and clk_bypass yet. That might change in the future
 as we iron out this brave new world that we all live in. Probably best
 to leave a FIXME in there.

 Stephen  Tomeu, let me know if I got any of that wrong.

The plan is to make clk_get_parent_by_index() return a clk_hw pointer
instead of a clk pointer (probably with a new
clk_get_parent_hw_by_index() API). Then drivers that are clk providers
can deal in struct clk_hw and clk consumers can deal in struct clk,
nicely splitting the API between consumers and providers on the
structures they use to interact with the framework.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/02/15 13:31, Julia Lawall wrote:

 On Mon, 2 Feb 2015, Stephen Boyd wrote:

 Julia,

 Is there a way we can write a coccinelle script to check for this? The
 goal being to find all drivers that are comparing struct clk pointers or
 attempting to dereference them. There are probably other frameworks that
 could use the same type of check (regulator, gpiod, reset, pwm, etc.).
 Probably anything that has a get/put API.
 Comparing or dereferencing pointers of a particular type should be 
 straightforward to check for.  Is there an example of how to use the 
 parent_index value to fix the problem?


I'm not sure how to fix this case with parent_index values generically.
I imagine it would be highly specific to the surrounding code to the
point where we couldn't fix it automatically. For example, if it's a clk
consumer (not provider like in this case) using an index wouldn't be the
right fix. We would need some sort of clk API that we don't currently
have and I would wonder why clock consumers even care to compare such
pointers in the first place.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Stephen Boyd]

On 01/29, Stephen Boyd wrote:
 On 01/29/15 05:31, Geert Uytterhoeven wrote:
  Hi Tomeu, Mike,
 
  On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
  tomeu.viz...@collabora.com wrote:
  --- a/drivers/clk/clk.c
  +++ b/drivers/clk/clk.c
  @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
  return 1;
   }
 
  -static void clk_core_put(struct clk_core *core)
  +void __clk_put(struct clk *clk)
   {
  struct module *owner;
 
  -   owner = core-owner;
  +   if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
  +   return;
 
  clk_prepare_lock();
  -   kref_put(core-ref, __clk_release);
  +
  +   hlist_del(clk-child_node);
  +   clk_core_set_rate_nolock(clk-core, clk-core-req_rate);
  At this point, clk-core-req_rate is still zero, causing
  cpg_div6_clock_round_rate() to be called with a zero rate parameter,
  e.g. on r8a7791:
 
 Hmm.. I wonder if we should assign core-req_rate to be the same as
 core-rate during __clk_init()? That would make this call to
 clk_core_set_rate_nolock() a nop in this case.
 

Here's a patch to do this

---8
From: Stephen Boyd sb...@codeaurora.org
Subject: [PATCH] clk: Assign a requested rate by default

We need to assign a requested rate here so that we avoid
requesting a rate of 0 on clocks when we remove clock consumers.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a29daf9edea4..8416ed1c40be 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2142,6 +2142,7 @@ int __clk_init(struct device *dev, struct clk *clk_user)
struct clk_core *orphan;
struct hlist_node *tmp2;
struct clk_core *clk;
+   unsigned long rate;
 
if (!clk_user)
return -EINVAL;
@@ -2266,12 +2267,13 @@ int __clk_init(struct device *dev, struct clk *clk_user)
 * then rate is set to zero.
 */
if (clk-ops-recalc_rate)
-   clk-rate = clk-ops-recalc_rate(clk-hw,
+   rate = clk-ops-recalc_rate(clk-hw,
clk_core_get_rate_nolock(clk-parent));
else if (clk-parent)
-   clk-rate = clk-parent-rate;
+   rate = clk-parent-rate;
else
-   clk-rate = 0;
+   rate = 0;
+   clk-rate = clk-req_rate = rate;
 
/*
 * walk the list of orphan clocks and reparent any that are children of
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Stephen Boyd]

On 01/29/15 05:31, Geert Uytterhoeven wrote:
 Hi Tomeu, Mike,

 On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
 tomeu.viz...@collabora.com wrote:
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
 return 1;
  }

 -static void clk_core_put(struct clk_core *core)
 +void __clk_put(struct clk *clk)
  {
 struct module *owner;

 -   owner = core-owner;
 +   if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
 +   return;

 clk_prepare_lock();
 -   kref_put(core-ref, __clk_release);
 +
 +   hlist_del(clk-child_node);
 +   clk_core_set_rate_nolock(clk-core, clk-core-req_rate);
 At this point, clk-core-req_rate is still zero, causing
 cpg_div6_clock_round_rate() to be called with a zero rate parameter,
 e.g. on r8a7791:

Hmm.. I wonder if we should assign core-req_rate to be the same as
core-rate during __clk_init()? That would make this call to
clk_core_set_rate_nolock() a nop in this case.


 cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 78000 div 1
 cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 78000 div 1
 cpg_div6_clock_round_rate: clock mmc0 rate 0 parent_rate 78000 div 1
 cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 78000 div 1
 cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 78000 div 1
 cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 78000 div 1
 cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 78000 div 1

 and cpg_div6_clock_calc_div() is called to calculate

 div = DIV_ROUND_CLOSEST(parent_rate, rate);

 Why was this call to clk_core_set_rate_nolock() in __clk_put() added?
 Before, there was no rate setting done at this point, and
 cpg_div6_clock_round_rate() was not called.

We need to call clk_core_set_rate_nolock() here to drop any min/max rate
request that this consumer has.


 Have the semantics changed? Should .round_rate() be ready to
 accept a zero rate, and use e.g. the current rate instead?

It seems like we've also exposed a bug in cpg_div6_clock_calc_div().
Technically any driver could have called clk_round_rate() with a zero
rate before this change and that would have caused the same division by
zero.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 2/4] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 01/22, Tomeu Vizoso wrote:
 On 01/22/2015 02:01 AM, Stephen Boyd wrote:
  BTW, please try and fixup checkpatch warnings.
 
 What were you thinking of specifically? I'm running it with
 --max-line-length=106 and the other warnings are in clk-test.c that I
 still have to polish when I get some time.

I can see that sometimes we exceed the 80 character limits that
are configured by default. We mostly stick to 80 in this file it
seems so I'm not sure why 106 is being used.

  
  And we do it here where we could remove the #ifdef.
 
 Yeah, I tried to reduce the ifdefing back then and this is the simplest
 I could come up with. The reason for clk_get() to call
 __clk_create_clk() directly is that it has more relevant information
 with which to tag the per-user clk.
 
 of_clk_get_by_name() has the name of the node but not the dev_id, which
 in my testing looked as much less useful when debugging who did what to
 a clock.
 

Agreed. But didn't we add __of_clk_get_by_name() so that we could
pass the dev_id and con_id to it? If we did that then all the
relevant information is there and we can call __clk_create_clk()
directly instead of relying on the caller to do it.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 4/6] clk: Add rate constraints to clocks

[Stephen Boyd]

On 01/22, Tomeu Vizoso wrote:
 Adds a way for clock consumers to set maximum and minimum rates. This
 can be used for thermal drivers to set minimum rates, or by misc.
 drivers to set maximum rates to assure a minimum performance level.
 
 Changes the signature of the determine_rate callback by adding the
 parameters min_rate and max_rate.
 
 Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com
 

Reviewed-by: Stephen Boyd sb...@codeaurora.org

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 01/22, Tomeu Vizoso wrote:
 Moves clock state to struct clk_core, but takes care to change as little API 
 as
 possible.
 
 struct clk_hw still has a pointer to a struct clk, which is the
 implementation's per-user clk instance, for backwards compatibility.
 
 The struct clk that clk_get_parent() returns isn't owned by the caller, but by
 the clock implementation, so the former shouldn't call clk_put() on it.
 
 Because some boards in mach-omap2 still register clocks statically, their 
 clock
 registration had to be updated to take into account that the clock information
 is stored in struct clk_core now.
 
 Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com
 
 ---

My comment on v11 still stands about __clk_create_clk() in
clkdev.c, but otherwise I don't see problems. You can have my
reviewed-by anyway.

Reviewed-by: Stephen Boyd sb...@codeaurora.org

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 2/4] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 01/21, Tomeu Vizoso wrote:
 @@ -2075,10 +2210,12 @@ struct clk *clk_register(struct device *dev, struct 
 clk_hw *hw)
   }
   }
  
 - ret = __clk_init(dev, clk);
 + hw-clk = __clk_create_clk(hw, NULL, NULL);
 + ret = __clk_init(dev, hw-clk);
   if (!ret)
 - return clk;
 + return hw-clk;
  
 + kfree(hw-clk);
  fail_parent_names_copy:
   while (--i = 0)
   kfree(clk-parent_names[i]);

Sigh, this patch is so huge I keep finding more things. Sorry. It
looks like __clk_create_clk() can return an error pointer, which
we then send directly to __clk_init. First off, we shouldn't
kfree() that pointer if it's an error pointer. Second, we
shouldn't crash in __clk_init() in such a situation so there
needs to be some sort of check somewhere.

BTW, please try and fixup checkpatch warnings.

 diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
 index da4bda8..fac3244 100644
 --- a/drivers/clk/clkdev.c
 +++ b/drivers/clk/clkdev.c
 @@ -69,20 +70,22 @@ struct clk *of_clk_get(struct device_node *np, int index)
[...]
 -struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
 +static struct clk *__of_clk_get_by_name(struct device_node *np, const char 
 *name)

It would be nice if this returned an already __clk_create_clk()ed
pointer.

  {
   struct clk *clk = ERR_PTR(-ENOENT);
  
 @@ -119,7 +122,33 @@ struct clk *of_clk_get_by_name(struct device_node *np, 
 const char *name)
[...]
 +struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
 +{
 + struct clk *clk = __of_clk_get_by_name(np, name);
 +
 + if (!IS_ERR(clk))
 + clk = __clk_create_clk(__clk_get_hw(clk), np-full_name, name);

Because we do it here where we know we're CONFIG_COMMON_CLK=y.

 +
 + return clk;
 +}
  EXPORT_SYMBOL(of_clk_get_by_name);
 +
 +#else /* defined(CONFIG_OF)  defined(CONFIG_COMMON_CLK) */
 +
 +static struct clk *__of_clk_get_by_name(struct device_node *np, const char 
 *name)
 +{
 + return ERR_PTR(-ENOENT);
 +}
  #endif
  
  /*
 @@ -185,9 +229,13 @@ struct clk *clk_get(struct device *dev, const char 
 *con_id)
   struct clk *clk;
  
   if (dev) {
 - clk = of_clk_get_by_name(dev-of_node, con_id);
 - if (!IS_ERR(clk))
 + clk = __of_clk_get_by_name(dev-of_node, con_id);
 + if (!IS_ERR(clk)) {
 +#if defined(CONFIG_COMMON_CLK)
 + clk = __clk_create_clk(__clk_get_hw(clk), dev_id, 
 con_id);
 +#endif

And we do it here where we could remove the #ifdef.

   return clk;
 + }
   if (PTR_ERR(clk) == -EPROBE_DEFER)
   return clk;
   }

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 3/4] clk: Add rate constraints to clocks

[Stephen Boyd]

On 01/21, Tomeu Vizoso wrote:
 Adds a way for clock consumers to set maximum and minimum rates. This
 can be used for thermal drivers to set minimum rates, or by misc.
 drivers to set maximum rates to assure a minimum performance level.
 
 Changes the signature of the determine_rate callback by adding the
 parameters min_rate and max_rate.
 
 Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com
 
 ---
 v11:  * Recalculate the rate before putting the reference to clk_core
   * Don't recalculate the rate when freeing the per-user clock
   in the initialization error paths
   * Move __clk_create_clk to be next to __clk_free_clk for more
   comfortable reading

Can we do this in the previous patch where we introduce the
function?

 @@ -2143,9 +2314,16 @@ struct clk *__clk_register(struct device *dev, struct 
 clk_hw *hw)
   else
   clk-owner = NULL;
  
 + INIT_HLIST_HEAD(clk-clks);
 +
 + hw-clk = __clk_create_clk(hw, NULL, NULL);
 +
   ret = __clk_init(dev, hw-clk);
 - if (ret)
 + if (ret) {
 + __clk_free_clk(hw-clk);
 + hw-clk = NULL;
   return ERR_PTR(ret);
 + }
  
   return hw-clk;
  }
 @@ -2210,12 +2388,16 @@ struct clk *clk_register(struct device *dev, struct 
 clk_hw *hw)
   }
   }
  
 + INIT_HLIST_HEAD(clk-clks);
 +
   hw-clk = __clk_create_clk(hw, NULL, NULL);
   ret = __clk_init(dev, hw-clk);
   if (!ret)
   return hw-clk;
  
 - kfree(hw-clk);
 + __clk_free_clk(hw-clk);
 + hw-clk = NULL;

Shouldn't we be assigning to NULL in the previous patch (same
comment for __clk_register)?

  fail_parent_names_copy:
   while (--i = 0)
   kfree(clk-parent_names[i]);
 @@ -2420,7 +2602,14 @@ void __clk_put(struct clk *clk)
   if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
   return;
  
 + clk_prepare_lock();
 + hlist_del(clk-child_node);
 + clk_prepare_unlock();
 +
 + clk_core_set_rate(clk-core, clk-core-req_rate);
 +
   clk_core_put(clk-core);
 +

Sad that we take the lock 3 times during __clk_put(). We should
be able to do it only once if we have a lockless
clk_core_set_rate() function and put the contents of
clk_core_put() into this function. Actually we need to do that to
be thread safe with clk-core-req_rate changing. We can call the
same function in clk_set_rate_range() too so that we don't have
to deal with recursive locking there.

   kfree(clk);
  }
  

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 2/3] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 01/20, Tomeu Vizoso wrote:
 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index 97f3425..e867d6a 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -1719,6 +1817,31 @@ out:
  
   return ret;
  }
 +
 +/**
 + * clk_set_parent - switch the parent of a mux clk
 + * @clk: the mux clk whose input we are switching
 + * @parent: the new input to clk
 + *
 + * Re-parent clk to use parent as its new input source.  If clk is in
 + * prepared state, the clk will get enabled for the duration of this call. If
 + * that's not acceptable for a specific clk (Eg: the consumer can't handle
 + * that, the reparenting is glitchy in hardware, etc), use the
 + * CLK_SET_PARENT_GATE flag to allow reparenting only when clk is unprepared.
 + *
 + * After successfully changing clk's parent clk_set_parent will update the
 + * clk topology, sysfs topology and propagate rate recalculation via
 + * __clk_recalc_rates.
 + *
 + * Returns 0 on success, -EERROR otherwise.
 + */
 +int clk_set_parent(struct clk *clk, struct clk *parent)
 +{
 + if (!clk || !parent)
 + return 0;

This is a behavior change, although it's very possible nobody
cares besides clk.c itself. Before this change
clk_set_parent(clk, NULL) would orphan the clock and move it to
the orphan list. Now we're going to do nothing. We should keep
the original behavior, although I don't know why anybody would
want to orphan a clock from a driver. The only place I think
we're using this correctly is on the clock unregistration path.

 +
 + return clk_core_set_parent(clk-core, parent-core);
 +}
  EXPORT_SYMBOL_GPL(clk_set_parent);
  
  /**

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 3/3] clk: Add rate constraints to clocks

[Stephen Boyd]

It's looking fairly close. Thanks for keeping up with the review
comments.

On 01/20, Tomeu Vizoso wrote:
 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index e867d6a..f241e27 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -2143,6 +2280,10 @@ struct clk *__clk_register(struct device *dev, struct 
 clk_hw *hw)
   else
   clk-owner = NULL;
  
 + INIT_HLIST_HEAD(clk-clks);
 +
 + hw-clk = __clk_create_clk(hw, NULL, NULL);
 +
   ret = __clk_init(dev, hw-clk);
   if (ret)
   return ERR_PTR(ret);

Don't we need to __clk_free_clk() here too?

 @@ -2151,6 +2292,19 @@ struct clk *__clk_register(struct device *dev, struct 
 clk_hw *hw)
  }
  EXPORT_SYMBOL_GPL(__clk_register);
  
 +static void __clk_free_clk(struct clk *clk)
 +{
 + struct clk_core *core = clk-core;
 +
 + clk_prepare_lock();
 + hlist_del(clk-child_node);
 + clk_prepare_unlock();
 +
 + kfree(clk);
 +
 + clk_core_set_rate(core, core-req_rate);

Is it safe to call this during clock registration? I hope that it
will just bail out and do nothing because core-rate ==
core-req_rate. Maybe we can avoid this given my next comment
below.

 +}
 +
  /**
   * clk_register - allocate a new clock, register it and return an opaque 
 cookie
   * @dev: device that is registering this clock
 @@ -2210,12 +2364,14 @@ struct clk *clk_register(struct device *dev, struct 
 clk_hw *hw)
   }
   }
  
 + INIT_HLIST_HEAD(clk-clks);
 +
   hw-clk = __clk_create_clk(hw, NULL, NULL);
   ret = __clk_init(dev, hw-clk);
   if (!ret)
   return hw-clk;
  
 - kfree(hw-clk);
 + __clk_free_clk(hw-clk);
  fail_parent_names_copy:
   while (--i = 0)
   kfree(clk-parent_names[i]);
 @@ -2421,7 +2577,7 @@ void __clk_put(struct clk *clk)
   return;
  
   clk_core_put(clk-core);
 - kfree(clk);
 + __clk_free_clk(clk);

This doesn't look right. First we drop the core reference here
with clk_core_put() and then we call __clk_free_clk() which will
go and call clk_core_set_rate() on the clk-core which may or may
not exist anymore. I'd think we want to do these steps:

 1. Unlink clk from clks list
 2. Recalculate rate and set if changed
 3. Drop kref on core with clk_core_put()
 4. kfree the clk

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 3/3] clk: Add floor and ceiling constraints to clock rates

[Stephen Boyd]

On 01/19, Tomeu Vizoso wrote:
 Adds a way for clock consumers to set maximum and minimum rates. This can be
 used for thermal drivers to set ceiling rates, or by misc. drivers to set
 floor rates to assure a minimum performance level.
 
 Changes the signature of the determine_rate callback by adding the
 parameters floor_rate and ceiling_rate.

Commit text needs the s/floor/min and s/ceiling/max treatment
too.

 
 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index f2a1ff3..55b3124 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -1026,6 +1051,28 @@ static unsigned long clk_core_round_rate_nolock(struct 
 clk_core *clk,
   else
   return clk-rate;
  }
 +unsigned long __clk_determine_rate(struct clk_hw *hw,
 +unsigned long rate,
 +unsigned long min_rate,
 +unsigned long max_rate)
 +{
 + unsigned long parent_rate = 0;
 + struct clk_core *core = hw-core;
 + struct clk_hw *parent_hw;
 +
 + if (!core-ops-determine_rate)
 + return 0;
 +
 + if (core-parent) {
 + parent_rate = core-parent-rate;
 + parent_hw = core-parent-hw;
 + }
 +
 + return core-ops-determine_rate(core-hw, rate,
 + min_rate, max_rate,
 + parent_rate, parent_hw);
 +}
 +EXPORT_SYMBOL_GPL(__clk_determine_rate);

Maybe I misled you with the API name. I was thinking more along
the lines of clk_round_rate() and this new function ending up
calling clk_core_round_rate(), but clk_round_rate() would call it
with whatever range the clock is constrained to while this new
function would allow driver authors to specify the range. It
should be easy enough to add min/max to clk_core_round_rate()
given that it's a private API in this file.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND v8 1/2] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 01/19/2015 01:55 AM, Tomeu Vizoso wrote:
 On 17 January 2015 at 02:02, Stephen Boyd sb...@codeaurora.org wrote:
 On 01/12, Tomeu Vizoso wrote:

 +}
  EXPORT_SYMBOL_GPL(__clk_get_rate);

 @@ -630,7 +656,12 @@ out:
   return !!ret;
  }

 -bool __clk_is_enabled(struct clk *clk)
 +bool __clk_is_prepared(struct clk *clk)
 +{
 + return clk_core_is_prepared(clk-core);
 Oops. clk can be NULL here. Return false if so. Or drop the
 function entirely? It looks like it may become unused.
 Are you thinking of anything specific that the alchemy arch can do
 instead of calling __clk_is_prepared?



Ah I missed that one. Bad grep.

 +}
  EXPORT_SYMBOL_GPL(__clk_is_enabled);

 @@ -762,7 +805,12 @@ void __clk_unprepare(struct clk *clk)
   if (clk-ops-unprepare)
   clk-ops-unprepare(clk-hw);

 - __clk_unprepare(clk-parent);
 + clk_core_unprepare(clk-parent);
 +}
 +
 +void __clk_unprepare(struct clk *clk)
 +{
 + clk_core_unprepare(clk-core);
 OOps. clk can be NULL here. Bail early if so.
 Actually, looks like nobody is using __clk_prepare nor __clk_unprepare
 so I'm removing these.


Ok.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 2/3] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 01/19, Tomeu Vizoso wrote:
 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index 97f3425..f2a1ff3 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -694,32 +751,32 @@ long __clk_mux_determine_rate(struct clk_hw *hw, 
 unsigned long rate,
 unsigned long *best_parent_rate,
 struct clk_hw **best_parent_p)
  {
 - struct clk *clk = hw-clk, *parent, *best_parent = NULL;
 + struct clk_core *core = hw-clk-core, *parent, *best_parent = NULL;

Can't we just use hw-core here?

   int i, num_parents;
   unsigned long parent_rate, best = 0;
  
  
 @@ -820,15 +877,18 @@ int clk_prepare(struct clk *clk)
  {
   int ret;
  
 + if (IS_ERR_OR_NULL(clk))
 + return PTR_ERR(clk);

What's going on here? Should be if (!clk)?

 +
   clk_prepare_lock();
 - ret = __clk_prepare(clk);
 + ret = clk_core_prepare(clk-core);
   clk_prepare_unlock();
  
   return ret;
  }
  EXPORT_SYMBOL_GPL(clk_prepare);
 @@ -1066,9 +1149,24 @@ long clk_get_accuracy(struct clk *clk)
  
   return accuracy;
  }
 +
 +/**
 + * clk_get_accuracy - return the accuracy of clk
 + * @clk: the clk whose accuracy is being returned
 + *
 + * Simply returns the cached accuracy of the clk, unless
 + * CLK_GET_ACCURACY_NOCACHE flag is set, which means a recalc_rate will be
 + * issued.
 + * If clk is NULL then returns 0.
 + */
 +long clk_get_accuracy(struct clk *clk)
 +{
 + return clk_core_get_accuracy(clk-core);

Oops. Missing NULL check.

 +}
  EXPORT_SYMBOL_GPL(clk_get_accuracy);
  
 @@ -1130,14 +1220,29 @@ unsigned long clk_get_rate(struct clk *clk)
[...]
 + *
 + * Simply returns the cached rate of the clk, unless CLK_GET_RATE_NOCACHE 
 flag
 + * is set, which means a recalc_rate will be issued.
 + * If clk is NULL then returns 0.
 + */
 +unsigned long clk_get_rate(struct clk *clk)
 +{
 + return clk_core_get_rate(clk-core);

Oops. Missing NULL check.

 +}
  EXPORT_SYMBOL_GPL(clk_get_rate);
 @@ -1629,37 +1741,26 @@ static struct clk *__clk_init_parent(struct clk *clk)
[...]
 -int clk_set_parent(struct clk *clk, struct clk *parent)
 +void __clk_reparent(struct clk *clk, struct clk *new_parent)
 +{
 + clk_core_reparent(clk-core, new_parent-core);
 +}

Is this used? Looks like we can remove it. Sorry, not sure how I
missed this last time.

 +
 +static int clk_core_set_parent(struct clk_core *clk, struct clk_core *parent)
  {
   int ret = 0;
   int p_index = 0;
 @@ -1719,6 +1820,28 @@ out:
[...]
 +int clk_set_parent(struct clk *clk, struct clk *parent)
 +{
 + return clk_core_set_parent(clk-core, parent-core);

Oops. Missing NULL check for both inputs.

 +}
  EXPORT_SYMBOL_GPL(clk_set_parent);
  
  /**
 @@ -1793,18 +1909,31 @@ out:
  }
  
  /**
 + * clk_get_phase - return the phase shift of a clock signal
 + * @clk: clock signal source
 + *
 + * Returns the phase shift of a clock node in degrees, otherwise returns
 + * -EERROR.
 + */
 +int clk_get_phase(struct clk *clk)
 +{
 + return clk_core_get_phase(clk-core);

Oops. Missing NULL check.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND v8 1/2] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 01/12, Tomeu Vizoso wrote:
 Moves clock state to struct clk_core, but takes care to change as little API 
 as
 possible.
 
 struct clk_hw still has a pointer to a struct clk, which is the
 implementation's per-user clk instance, for backwards compatibility.
 
 The struct clk that clk_get_parent() returns isn't owned by the caller, but by
 the clock implementation, so the former shouldn't call clk_put() on it.
 
 Because some boards in mach-omap2 still register clocks statically, their 
 clock
 registration had to be updated to take into account that the clock information
 is stored in struct clk_core now.
 
 Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com
 

Looks mostly good. Missing some NULL checks mostly.

 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index f4963b7..7eddfd8 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -114,7 +123,7 @@ static struct hlist_head *orphan_list[] = {
 +static void clk_summary_show_one(struct seq_file *s, struct clk_core *c, int 
 level)
  {
   if (!c)
   return;
 @@ -122,14 +131,14 @@ static void clk_summary_show_one(struct seq_file *s, 
 struct clk *c, int level)
[...]
 -static void clk_summary_show_subtree(struct seq_file *s, struct clk *c,
 +static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,
int level)
  {
 - struct clk *child;
 + struct clk_core *child;
  
   if (!c)
   return;
 @@ -172,7 +181,7 @@ static const struct file_operations clk_summary_fops = {
   .release= single_release,
  };
  
 -static void clk_dump_one(struct seq_file *s, struct clk *c, int level)
 +static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
  {
   if (!c)
   return;
 @@ -180,14 +189,14 @@ static void clk_dump_one(struct seq_file *s, struct clk 
 *c, int level)
[...]
 -static void clk_dump_subtree(struct seq_file *s, struct clk *c, int level)
 +static void clk_dump_subtree(struct seq_file *s, struct clk_core *c, int 
 level)
  {
 - struct clk *child;
 + struct clk_core *child;
  
   if (!c)
   return;
 @@ -418,19 +427,20 @@ static int __init clk_debug_init(void)
[...]
  
  /* caller must hold prepare_lock */
 -static void clk_unprepare_unused_subtree(struct clk *clk)
 +static void clk_unprepare_unused_subtree(struct clk_core *clk)
  {
 - struct clk *child;
 + struct clk_core *child;
  
   if (!clk)
   return;
 @@ -453,9 +463,9 @@ static void clk_unprepare_unused_subtree(struct clk *clk)
  }
  
  /* caller must hold prepare_lock */
 -static void clk_disable_unused_subtree(struct clk *clk)
 +static void clk_disable_unused_subtree(struct clk_core *clk)
  {
 - struct clk *child;
 + struct clk_core *child;
   unsigned long flags;
  
   if (!clk)

Note: These NULL checks look bogus. No need to fix them here, but
a patch to remove them would be nice.

 @@ -532,48 +542,59 @@ late_initcall_sync(clk_disable_unused);
[...]
 +
 +struct clk *clk_get_parent_by_index(struct clk *clk, u8 index)
 +{
 + struct clk_core *parent;
 +
 + parent = clk_core_get_parent_by_index(clk-core, index);

I suppose clk could be NULL here (although this is mostly a
clk-provider function). At least before we would return NULL in
such a case so we should keep the same behavior instead of NULL
deref.

 +
 + return !parent ? NULL : parent-hw-clk;
 +}
  EXPORT_SYMBOL_GPL(clk_get_parent_by_index);
  
 @@ -593,9 +614,14 @@ unsigned long __clk_get_rate(struct clk *clk)
  out:
   return ret;
  }
 +
 +unsigned long __clk_get_rate(struct clk *clk)
 +{
 + return clk_core_get_rate_nolock(clk-core);

Oops. clk can be NULL here. We should check for that and return
0.

 +}
  EXPORT_SYMBOL_GPL(__clk_get_rate);
  
 @@ -630,7 +656,12 @@ out:
   return !!ret;
  }
  
 -bool __clk_is_enabled(struct clk *clk)
 +bool __clk_is_prepared(struct clk *clk)
 +{
 + return clk_core_is_prepared(clk-core);

Oops. clk can be NULL here. Return false if so. Or drop the
function entirely? It looks like it may become unused.

 +}
 @@ -650,12 +681,17 @@ bool __clk_is_enabled(struct clk *clk)
  out:
   return !!ret;
  }
 +
 +bool __clk_is_enabled(struct clk *clk)
 +{
 + return clk_core_is_enabled(clk-core);

Oops. clk can be NULL here. Return false if so.

 +}
  EXPORT_SYMBOL_GPL(__clk_is_enabled);
  
 @@ -762,7 +805,12 @@ void __clk_unprepare(struct clk *clk)
   if (clk-ops-unprepare)
   clk-ops-unprepare(clk-hw);
  
 - __clk_unprepare(clk-parent);
 + clk_core_unprepare(clk-parent);
 +}
 +
 +void __clk_unprepare(struct clk *clk)
 +{
 + clk_core_unprepare(clk-core);

OOps. clk can be NULL here. Bail early if so.

  }
  
  /**
 @@ -813,6 +861,11 @@ int __clk_prepare(struct clk *clk)
   return 0;
  }
  
 +int __clk_prepare(struct clk *clk)
 +{
 + return clk_core_prepare(clk-core);

Oops. clk can be NULL here. Return 0 if so.

 +}
 +
 @@ -851,7 +904,12 @@ 

Re: [PATCH RESEND v8 2/2] clk: Add floor and ceiling constraints to clock rates

[Stephen Boyd]

On 01/12, Tomeu Vizoso wrote:
 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index 7eddfd8..2793bd7 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -1013,8 +1015,8 @@ static unsigned long clk_core_round_rate_nolock(struct 
 clk_core *clk,
  
   if (clk-ops-determine_rate) {
   parent_hw = parent ? parent-hw : NULL;
 - return clk-ops-determine_rate(clk-hw, rate, parent_rate,
 - parent_hw);
 + return clk-ops-determine_rate(clk-hw, rate, 0, ULONG_MAX,
 + parent_rate, parent_hw);
   } else if (clk-ops-round_rate)
   return clk-ops-round_rate(clk-hw, rate, parent_rate);
   else if (clk-flags  CLK_SET_RATE_PARENT)
 @@ -1453,8 +1458,20 @@ static struct clk_core *clk_calc_new_rates(struct 
 clk_core *clk,
  
   /* find the closest rate and parent clk/rate */
   if (clk-ops-determine_rate) {
 + hlist_for_each_entry(clk_user, clk-clks, child_node) {
 + floor_rate = max(floor_rate,
 +  clk_user-floor_constraint);
 + }
 +
 + hlist_for_each_entry(clk_user, clk-clks, child_node) {
 + ceiling_rate = min(ceiling_rate,
 +clk_user-ceiling_constraint);
 + }

I would think we need to do this in the clk_round_rate() path as
well. We can't just pass 0 and ULONG_MAX there or we'll determine
one rate here and another rate in round_rate(), violating the
contract between set_rate() and round_rate().

 +
   parent_hw = parent ? parent-hw : NULL;
   new_rate = clk-ops-determine_rate(clk-hw, rate,
 + floor_rate,
 + ceiling_rate,
   best_parent_rate,
   parent_hw);
   parent = parent_hw ? parent_hw-core : NULL;

We should enforce a constraint if the clk is using the
round_rate() op too. If the .round_rate() op returns some rate
within range it should be ok.  Otherwise we can fail the rate
change because it's out of range.

We'll also need to introduce some sort of
clk_core_determine_rate(core, rate, min, max) so that clock
providers can ask parent clocks to find a rate within some range
that they can tolerate. If we update __clk_mux_determine_rate()
we can see how that would work out.

 @@ -1660,13 +1657,92 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
[...]
 + */
 +int clk_set_rate(struct clk *clk, unsigned long rate)
 +{
 + return clk_core_set_rate(clk-core, rate);

clk could be NULL.

 +}
  EXPORT_SYMBOL_GPL(clk_set_rate);
  
 +int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 +{
 + int ret = 0;

Check for NULL clk.

 +
 +/**
 + * clk_set_floor_rate - set a minimum clock rate for a clock source
 + * @clk: clock source
 + * @rate: desired minimum clock rate in Hz
 + *
 + * Returns success (0) or negative errno.
 + */
 +int clk_set_floor_rate(struct clk *clk, unsigned long rate)
 +{
 + return clk_set_rate_range(clk, rate, clk-ceiling_constraint);

clk could be NULL.

 +}
 +EXPORT_SYMBOL_GPL(clk_set_floor_rate);
 +
 +/**
 + * clk_set_ceiling_rate - set a maximum clock rate for a clock source
 + * @clk: clock source
 + * @rate: desired maximum clock rate in Hz
 + *
 + * Returns success (0) or negative errno.
 + */
 +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate)
 +{
 + return clk_set_rate_range(clk, clk-floor_constraint, rate);

clk could be NULL.

 +}
 +EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);
 +
  static struct clk_core *clk_core_get_parent(struct clk_core *core)
  {
   struct clk_core *parent;
 diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
 index 2e65419..ae5c800 100644
 --- a/include/linux/clk-provider.h
 +++ b/include/linux/clk-provider.h
 @@ -175,9 +175,12 @@ struct clk_ops {
   unsigned long parent_rate);
   long(*round_rate)(struct clk_hw *hw, unsigned long rate,
   unsigned long *parent_rate);
 - long(*determine_rate)(struct clk_hw *hw, unsigned long rate,
 - unsigned long *best_parent_rate,
 - struct clk_hw **best_parent_hw);
 + long(*determine_rate)(struct clk_hw *hw,
 +   unsigned long rate,
 +   unsigned long floor_rate,
 +   unsigned long ceiling_rate,

I wonder if we should call this min_rate and max_rate?

 +   unsigned long *best_parent_rate,
 +   struct clk_hw **best_parent_hw);
   int 

Re: regression: Clock changes in next-20141205 break at least omap4

[Stephen Boyd]

On 12/15/2014 05:31 PM, Paul Walmsley wrote:

 I just took a quick glance at Tero's second patch, and it looks like a
 hack to me.  Better to fix the problem in the core CCF code if
 possible.  I don't think there's any reason why a PLL couldn't have
 just one parent clock.  But I'm fine with merging it as a short-term
 fix if fixing the core code is difficult or risky.

Can you describe what's wrong? Does the PLL have a mux with two inputs
that map to the same clock?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: Clock changes in next-20141205 break at least omap4

[Stephen Boyd]

On 12/05/2014 08:55 AM, Tony Lindgren wrote:
 Hi,

 Looks like commit 646cafc6aa4d (clk: Change clk_ops-determine_rate
 to return a clk_hw as the best parent) breaks booting at least for
 omap4.

Do you get a compilation warning in arch/arm/mach-omap2/dpll3xxx.c ?
From what I can tell omap3_noncore_dpll_determine_rate() hasn't been
updated to take a clk_hw pointer instead of clk pointer. It was there in
the original patch and I'm not sure why Mike dropped that part while
applying.

diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
index 20e120d..c2da2a0 100644
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -474,7 +474,7 @@ void omap3_noncore_dpll_disable(struct clk_hw *hw)
  */
 long omap3_noncore_dpll_determine_rate(struct clk_hw *hw, unsigned long rate,
   unsigned long *best_parent_rate,
-  struct clk **best_parent_clk)
+  struct clk_hw **best_parent_clk)
 {
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
struct dpll_data *dd;
@@ -488,10 +488,10 @@ long omap3_noncore_dpll_determine_rate(struct clk_hw *hw, 
unsigned long rate,
 
if (__clk_get_rate(dd-clk_bypass) == rate 
(dd-modes  (1  DPLL_LOW_POWER_BYPASS))) {
-   *best_parent_clk = dd-clk_bypass;
+   *best_parent_clk = __clk_get_hw(dd-clk_bypass);
} else {
rate = omap2_dpll_round_rate(hw, rate, best_parent_rate);
-   *best_parent_clk = dd-clk_ref;
+   *best_parent_clk = __clk_get_hw(dd-clk_ref);
}
 
*best_parent_rate = rate;
diff --git a/arch/arm/mach-omap2/dpll44xx.c b/arch/arm/mach-omap2/dpll44xx.c
index 535822f..0e58e5a 100644
--- a/arch/arm/mach-omap2/dpll44xx.c
+++ b/arch/arm/mach-omap2/dpll44xx.c
@@ -223,7 +223,7 @@ out:
  */
 long omap4_dpll_regm4xen_determine_rate(struct clk_hw *hw, unsigned long rate,
unsigned long *best_parent_rate,
-   struct clk **best_parent_clk)
+   struct clk_hw **best_parent_clk)
 {
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
struct dpll_data *dd;
@@ -237,11 +237,11 @@ long omap4_dpll_regm4xen_determine_rate(struct clk_hw 
*hw, unsigned long rate,
 
if (__clk_get_rate(dd-clk_bypass) == rate 
(dd-modes  (1  DPLL_LOW_POWER_BYPASS))) {
-   *best_parent_clk = dd-clk_bypass;
+   *best_parent_clk = __clk_get_hw(dd-clk_bypass);
} else {
rate = omap4_dpll_regm4xen_round_rate(hw, rate,
  best_parent_rate);
-   *best_parent_clk = dd-clk_ref;
+   *best_parent_clk = __clk_get_hw(dd-clk_ref);
}
 
*best_parent_rate = rate;

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 6/7] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 10/30, Tomeu Vizoso wrote:
 Moves clock state to struct clk_core, but takes care to change as little API 
 as
 possible.
 
 struct clk_hw still has a pointer to a struct clk, which is the
 implementation's per-user clk instance, for backwards compatibility.
 
 The struct clk that clk_get_parent() returns isn't owned by the caller, but by
 the clock implementation, so the former shouldn't call clk_put() on it.
 
 Because some boards in mach-omap2 still register clocks statically, their 
 clock
 registration had to be updated to take into account that the clock information
 is stored in struct clk_core now.
 
 Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com

It would be good to have Russell at least ack the clkdev bits.
There's still more work to do in the future here, but

Reviewed-by: Stephen Boyd sb...@codeaurora.org

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/8] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 10/09, Tomeu Vizoso wrote:
  arch/arm/mach-omap2/cclock3xxx_data.c   | 108 --
  arch/arm/mach-omap2/clock.h |  11 +-
  arch/arm/mach-omap2/clock_common_data.c |   5 +-
  drivers/clk/clk.c   | 630 
 
  drivers/clk/clk.h   |   5 +
  drivers/clk/clkdev.c|  24 +-
  include/linux/clk-private.h |  35 +-
  include/linux/clk-provider.h|  22 +-
  8 files changed, 549 insertions(+), 291 deletions(-)

The difstat looks good.

 
 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index fb820bf..4db918a 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -695,6 +731,13 @@ struct clk *__clk_lookup(const char *name)
   return NULL;
  }
  
 +struct clk *__clk_lookup(const char *name)
 +{
 + struct clk_core *clk = clk_core_lookup(name);
 +
 + return !clk ? NULL : clk-hw-clk;

This just looks weird with clk-hw-clk. I know we're trying to
keep the diff small by not renaming clk to core when it's used
extensively throughout the code, but for small little additions
like this I would prefer we use core for clk_core pointers and
clk for clk pointers. Then a patch at the end can rename
everything to be consistent. This thing also threw me off because
I searched for kfree(core) but couldn't find it so I thought we
leaked the clk_core structure.

 +}
 +
  /*
   * Helper for finding best parent to provide a given frequency. This can be 
 used
   * directly as a determine_rate callback (e.g. for a mux), or from a more
 @@ -2175,24 +2298,24 @@ void clk_unregister(struct clk *clk)
* a reference to this clock.
*/
   flags = clk_enable_lock();
 - clk-ops = clk_nodrv_ops;
 + clk-core-ops = clk_nodrv_ops;
   clk_enable_unlock(flags);
  
 - if (!hlist_empty(clk-children)) {
 - struct clk *child;
 + if (!hlist_empty(clk-core-children)) {
 + struct clk_core *child;
   struct hlist_node *t;
  
   /* Reparent all children to the orphan list. */
 - hlist_for_each_entry_safe(child, t, clk-children, child_node)
 - clk_set_parent(child, NULL);
 + hlist_for_each_entry_safe(child, t, clk-core-children, 
 child_node)
 + clk_core_set_parent(child, NULL);
   }
  
 - hlist_del_init(clk-child_node);
 + hlist_del_init(clk-core-child_node);
  
 - if (clk-prepare_count)
 + if (clk-core-prepare_count)
   pr_warn(%s: unregistering prepared clock: %s\n,
 - __func__, clk-name);
 - kref_put(clk-ref, __clk_release);
 + __func__, clk-core-name);
 + kref_put(clk-core-ref, __clk_release);
  
   clk_prepare_unlock();

It might be worth it to make a core local variable in this
function.

  }
 @@ -2255,32 +2378,38 @@ void devm_clk_unregister(struct device *dev, struct 
 clk *clk)
  }
  EXPORT_SYMBOL_GPL(devm_clk_unregister);
  
 +static void clk_core_put(struct clk_core *clk)
 +{
 + struct module *owner;
 +
 + if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
 + return;
 +
 + clk_prepare_lock();
 + owner = clk-owner;

Same here too, we don't need to protect the access to owner so it
can move outside the lock.

 + kref_put(clk-ref, __clk_release);
 + module_put(owner);
 + clk_prepare_unlock();
 +}
 +
  /*
   * clkdev helpers
   */
  int __clk_get(struct clk *clk)
  {
   if (clk) {
 - if (!try_module_get(clk-owner))
 + if (!try_module_get(clk-core-owner))
   return 0;
  
 - kref_get(clk-ref);
 + kref_get(clk-core-ref);
   }
   return 1;

Grow a core pointer?

  }
 @@ -2391,6 +2520,31 @@ int clk_notifier_unregister(struct clk *clk, struct 
 notifier_block *nb)
  }
  EXPORT_SYMBOL_GPL(clk_notifier_unregister);
  
 +struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,

Curious, why the underscore?

 +  const char *con_id)
 +{
 + struct clk *clk;
 +
 + /* This is to allow this function to be chained to others */
 + if (!hw || IS_ERR(hw))
 + return (struct clk *) hw;
 +
 + clk = kzalloc(sizeof(*clk), GFP_KERNEL);
 + if (!clk)
 + return ERR_PTR(-ENOMEM);
 +
 + clk-core = hw-core;
 + clk-dev_id = dev_id;
 + clk-con_id = con_id;
 +
 + return clk;
 +}
 diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
 index da4bda8..4411db6 100644
 --- a/drivers/clk/clkdev.c
 +++ b/drivers/clk/clkdev.c
 @@ -69,6 +70,10 @@ struct clk *of_clk_get(struct device_node *np, int index)
  
   clk = of_clk_get_by_clkspec(clkspec);
   of_node_put(clkspec.np);
 +
 + if (!IS_ERR(clk))
 + clk = __clk_create_clk(__clk_get_hw(clk), np-full_name, NULL);

We lost the debugging information here about the device
requesting this clock and the name they called it. 

Re: [PATCH v2 0/8] Per-user clock constraints

[Stephen Boyd]

On 10/07/2014 08:21 AM, Tomeu Vizoso wrote:

Hello,

this second version of the series adds several cleanups that were suggested by
Stephen Boyd and contains several improvements to the seventh patch (clk: Make
clk API return per-user struct clk instances) that were suggested by him during
the review of v1.

The first six patches are just cleanups that should be desirable on their own,
and that should make easier to review the actual per-user clock patch.

The seventh patch actually moves the per-clock data that was stored in struct
clk to a new struct clk_core and adds references to it from both struct clk and
struct clk_hw. struct clk is now ready to contain information that is specific
to a given clk consumer.

The eighth patch adds API for setting floor and ceiling constraints and stores
that information on the per-user struct clk, which is iterable from struct
clk_core.




As said in the patches, can you please indicate which baseline this is 
on? Also can you rebase onto clk-next if you send again before that is 
merged into 3.18-rc1? There are some changes in the debugfs part that 
will conflict. I'll review the more complicated parts in detail soon.


--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 10/07/2014 08:28 AM, Tomeu Vizoso wrote:

On 4 October 2014 01:15, Stephen Boyd sb...@codeaurora.org wrote:

On 10/02, Tomeu Vizoso wrote:

+
  #if defined(CONFIG_OF)  defined(CONFIG_COMMON_CLK)

These ifdefs look useless.


  struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec);
  struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
  void of_clk_lock(void);
  void of_clk_unlock(void);
  #endif
+
+#if defined(CONFIG_COMMON_CLK)

So we shouldn't need this one either.

Actually, i had to put it back so clkdev.c builds on
!CONFIG_COMMON_CLK. Do you have another idea on how to deal with this?


What's the error? Probably need to add a forward declaration for struct 
clk, struct of_phandle_args, and struct clk_hw.


--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 10/06/2014 10:14 AM, Tomeu Vizoso wrote:

This is the end goal. I understand that the provider API is sort
of a mess with us allowing drivers to use the underscore and
non-underscore functions and the mixture of struct clk and struct
ckl_hw throughout.

  struct clk_hw -- struct clk_core  struct clk
\- struct clk
|- struct clk

Agree this is how it should look like at some point, but for now I
need a reference to struct clk from struct clk_hw, so providers can
keep using the existing API. This reference would be removed once they
move to the new clk_hw-based API.


Ok sounds like we're on the same page.


+struct clk *__clk_create_clk(struct clk_core *clk_core, const char *dev_id,
+  const char *con_id);
+#endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index da4bda8..fe3712f 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -168,14 +172,27 @@ static struct clk_lookup *clk_find(const char *dev_id, 
const char *con_id)
  struct clk *clk_get_sys(const char *dev_id, const char *con_id)
  {
   struct clk_lookup *cl;
+ struct clk *clk = NULL;

   mutex_lock(clocks_mutex);
   cl = clk_find(dev_id, con_id);
- if (cl  !__clk_get(cl-clk))
- cl = NULL;
+ if (cl) {
+#if defined(CONFIG_COMMON_CLK)
+ clk = __clk_create_clk(cl-clk-core, dev_id, con_id);
+ if (clk  !__clk_get(clk)) {
+ kfree(clk);

This looks weird. It makes me suspect we've failed to reference
count something properly. Can we avoid this?

Can you extend on this? But I see how the behaviour doesn't match the
previous one because cl should be NULLed when __clk_get fails. I have
fixed this.


It triggers my that's not symmetric filter because it requires the 
caller to free something allocated by the callee. Do we still need 
__clk_get() to be called in the common clock path? Why not just do the 
stuff we do in __clk_get() in __clk_create_clk()? Then if that fails we 
can return an error pointer indicating some sort of failure (-ENOENT?) 
and we don't need to do any sort of cleanup otherwise.


--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 10/02, Tomeu Vizoso wrote:
 Moves clock state to struct clk_core, but takes care to change as little API 
 as
 possible.
 
 struct clk_hw still has a pointer to a struct clk, which is the
 implementation's per-user clk instance, for backwards compatibility.
 
 The struct clk that clk_get_parent() returns isn't owned by the caller, but by
 the clock implementation, so the former shouldn't call clk_put() on it.
 
 Because some boards in mach-omap2 still register clocks statically, their 
 clock
 registration had to be updated to take into account that the clock information
 is stored in struct clk_core now.
 
 Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com
 ---


We should s/provider/core/ when we're dealing with clk_core
structures in the function signature. The providers are hardware
drivers and the core structures are for the framework, not the
same. Furthermore, the provider drivers should only be dealing
with clk_hw structures. The only place that clk_core should be in
clk-provider.h is in struct clk_hw because there's no way to get
around it.

This way, provider drivers should only be including
clk-provider.h because they only care about dealing with struct
clk_hw. Consumers should only be including linux/clk.h where they
only know about struct clk as an opaque pointer. Once we get OMAP
to stop using clk-private.h we can kill off that header entirely
(I see there are some other bogus users of that header outside of
OMAP that we should nuke). Then the framework can include
clk-provider.h and clk.h to map between the hw cookie and the
consumer cookie.

This is the end goal. I understand that the provider API is sort
of a mess with us allowing drivers to use the underscore and
non-underscore functions and the mixture of struct clk and struct
ckl_hw throughout.

 struct clk_hw -- struct clk_core  struct clk
   \- struct clk
   |- struct clk

 providers
 -
 struct clk_hw {
struct clk_core *
...
 };

 consumers
 -

 struct clk;

 hidden in core framework
 
 struct clk {
struct clk_core *;
...
 }

 struct clk_core {
struct clk_hw *;
...
 }


 
 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index 4eeb8de..b216b13 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -37,6 +37,13 @@ static HLIST_HEAD(clk_root_list);
  static HLIST_HEAD(clk_orphan_list);
  static LIST_HEAD(clk_notifier_list);
  
 +static void clk_provider_put(struct clk_core *clk);

Does this need to be forward declared?

 +static long clk_provider_get_accuracy(struct clk_core *clk);
 +static bool clk_provider_is_prepared(struct clk_core *clk);
 +static bool clk_provider_is_enabled(struct clk_core *clk);
 +static long clk_provider_round_rate(struct clk_core *clk, unsigned long 
 rate);
 @@ -356,7 +363,7 @@ out:
   *
   * Caller must hold prepare_lock.
   */
 -static void clk_debug_unregister(struct clk *clk)
 +static void clk_debug_unregister(struct clk_core *clk)
  {
   debugfs_remove_recursive(clk-dentry);
  }
 @@ -366,8 +373,8 @@ struct dentry *clk_debugfs_add_file(struct clk *clk, char 
 *name, umode_t mode,

We should pass struct clk_hw here instead of struct clk. Let's do
it soon before we get any users.

  {
   struct dentry *d = NULL;
  
 - if (clk-dentry)
 - d = debugfs_create_file(name, mode, clk-dentry, data, fops);
 + if (clk-core-dentry)
 + d = debugfs_create_file(name, mode, clk-core-dentry, data, 
 fops);
  
   return d;
  }
 @@ -545,53 +553,67 @@ late_initcall_sync(clk_disable_unused);
  
  const char *__clk_get_name(struct clk *clk)
  {
 - return !clk ? NULL : clk-name;
 + return !clk ? NULL : clk-core-name;
  }
  EXPORT_SYMBOL_GPL(__clk_get_name);
  
  struct clk_hw *__clk_get_hw(struct clk *clk)
  {
 - return !clk ? NULL : clk-hw;
 + return !clk ? NULL : clk-core-hw;
  }
  EXPORT_SYMBOL_GPL(__clk_get_hw);
  
  u8 __clk_get_num_parents(struct clk *clk)
  {
 - return !clk ? 0 : clk-num_parents;
 + return !clk ? 0 : clk-core-num_parents;
  }
  EXPORT_SYMBOL_GPL(__clk_get_num_parents);
  
  struct clk *__clk_get_parent(struct clk *clk)
  {
 - return !clk ? NULL : clk-parent;
 + /* TODO: Create a per-user clk and change callers to call clk_put */

More like replace all callers with a function that returns their
parent's hw pointer.

struct clk_hw *clk_provider_get_parent(struct clk_hw *hw)


 + return !clk ? NULL : clk-core-parent-hw-clk;
  }
  EXPORT_SYMBOL_GPL(__clk_get_parent);
  
 -struct clk *clk_get_parent_by_index(struct clk *clk, u8 index)
 +static struct clk_core *clk_provider_get_parent_by_index(struct clk_core 
 *clk,
 +  u8 index)
  {
   if (!clk || index = clk-num_parents)
   return NULL;
   else if (!clk-parents)
 - return __clk_lookup(clk-parent_names[index]);
 + 

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Stephen Boyd]

On 09/23/14 06:38, Tero Kristo wrote:
 On 09/22/2014 10:18 PM, Stephen Boyd wrote:
 On 08/21, Tero Kristo wrote:
   /* Skip children who will be reparented to another clock */
   if (child-new_parent  child-new_parent != clk)
   continue;

 Are we not hitting the new_parent check here? I don't understand
 how we can be changing parents here unless the check is being
 avoided, in which case I wonder why determine_rate isn't being
 used.


 It depends how the clock underneath handles the situation. The error I
 am seeing actually happens with a SoC specific compound clock (DPLL)
 which integrates set_rate + mux functionality into a single clock
 node. A call to the clk_set_rate changes the parent of this clock
 (from bypass clock to reference clock), in addition to changing the
 rate (tune the mul+div.) I looked at using the determine rate call
 with this type but it breaks everything up... the parent gets changed
 but not the clock rate, in addition to some other issues.

Ok. Is this omap3_noncore_dpll_set_rate()? Can we use determine_rate +
clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
allow us to do the mult+div and the parent in the same op call, although
I don't understand why setting the parent and then setting the rate is
not going to work. I'm interested in the other issues that you mentioned
too.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Stephen Boyd]

On 08/21, Tero Kristo wrote:
 In some cases, clocks can switch their parent with clk_set_rate, for
 example clk_mux can do this in some cases. Current implementation of
 clk_change_rate uses un-safe list iteration on the clock children, which
 will cause wrong clocks to be parsed in case any of the clock children
 change their parents during the change rate operation. Fixed by using
 the safe list iterator instead.
 
 The problem was detected due to some divide by zero errors generated
 by clock init on dra7-evm board, see discussion under
 http://article.gmane.org/gmane.linux.ports.arm.kernel/349180 for details.
 
 Signed-off-by: Tero Kristo t-kri...@ti.com
 To: Mike Turquette mturque...@linaro.org
 Reported-by: Nishanth Menon n...@ti.com
 ---
  drivers/clk/clk.c |7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index b76fa69..bacc06f 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -1467,6 +1467,7 @@ static struct clk *clk_propagate_rate_change(struct clk 
 *clk, unsigned long even
  static void clk_change_rate(struct clk *clk)
  {
   struct clk *child;
 + struct hlist_node *tmp;
   unsigned long old_rate;
   unsigned long best_parent_rate = 0;
   bool skip_set_rate = false;
 @@ -1502,7 +1503,11 @@ static void clk_change_rate(struct clk *clk)
   if (clk-notifier_count  old_rate != clk-rate)
   __clk_notify(clk, POST_RATE_CHANGE, old_rate, clk-rate);
  
 - hlist_for_each_entry(child, clk-children, child_node) {
 + /*
 +  * Use safe iteration, as change_rate can actually swap parents
 +  * for certain clock types.
 +  */
 + hlist_for_each_entry_safe(child, tmp, clk-children, child_node) {
   /* Skip children who will be reparented to another clock */
   if (child-new_parent  child-new_parent != clk)
   continue;

Are we not hitting the new_parent check here? I don't understand
how we can be changing parents here unless the check is being
avoided, in which case I wonder why determine_rate isn't being
used.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 01/26] genirq: add irq_domain-aware core IRQ handler

[Stephen Boyd]

On 08/26/14 03:03, Marc Zyngier wrote:
 Calling irq_find_mapping from outside a irq_{enter,exit} section is
 unsafe and produces ugly messages if CONFIG_PROVE_RCU is enabled:
 If coming from the idle state, the rcu_read_lock call in irq_find_mapping
 will generate an unpleasant warning:

 quote
 ===
 [ INFO: suspicious RCU usage. ]
 3.16.0-rc1+ #135 Not tainted
 ---
 include/linux/rcupdate.h:871 rcu_read_lock() used illegally while idle!

 other info that might help us debug this:

 RCU used illegally from idle CPU!
 rcu_scheduler_active = 1, debug_locks = 0
 RCU used illegally from extended quiescent state!
 1 lock held by swapper/0/0:
  #0:  (rcu_read_lock){..}, at: [ffc00010206c]
 irq_find_mapping+0x4c/0x198

Do you have the whole stacktrace? I don't see where this is called
outside of irq_enter() from within the idle loop, but maybe I missed
something.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 01/26] genirq: add irq_domain-aware core IRQ handler

[Stephen Boyd]

On 08/26/14 11:07, Marc Zyngier wrote:
 Digging into my email, one of the traces looked like this:

 stack backtrace:
 CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.16.0-rc1+ #135
 Call trace:
 [ffc882cc] dump_backtrace+0x0/0x12c
 [ffc88408] show_stack+0x10/0x1c
 [ffc0004ee5f0] dump_stack+0x74/0xc4
 [ffcedfbc] lockdep_rcu_suspicious+0xe8/0x124
 [ffc00010218c] irq_find_mapping+0x16c/0x198
 [ffc8130c] gic_handle_irq+0x38/0xcc

 Most drivers call irq_find_mapping outside of irq_enter()/irq_exit(), as
 this is in handle_IRQ().


Ah ok. This is the multi-irq handler case? Has this been broken since
v3.2 at least for the gic users? Now that we call irq_enter()/irq_exit()
a lot more code runs, including things like updating jiffies when
interrupts arrive and invoking softirq? Do we only call irq_exit() on
the IPI path otherwise?

Are there any plans to send this back to stable trees? Not calling
irq_enter()/irq_exit() when we get an interrupt seems like a big problem.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 01/26] genirq: add irq_domain-aware core IRQ handler

[Stephen Boyd]

On 08/26/14 11:46, Stephen Boyd wrote:
 On 08/26/14 11:07, Marc Zyngier wrote:
 Digging into my email, one of the traces looked like this:

 stack backtrace:
 CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.16.0-rc1+ #135
 Call trace:
 [ffc882cc] dump_backtrace+0x0/0x12c
 [ffc88408] show_stack+0x10/0x1c
 [ffc0004ee5f0] dump_stack+0x74/0xc4
 [ffcedfbc] lockdep_rcu_suspicious+0xe8/0x124
 [ffc00010218c] irq_find_mapping+0x16c/0x198
 [ffc8130c] gic_handle_irq+0x38/0xcc

 Most drivers call irq_find_mapping outside of irq_enter()/irq_exit(), as
 this is in handle_IRQ().

 Ah ok. This is the multi-irq handler case? Has this been broken since
 v3.2 at least for the gic users? Now that we call irq_enter()/irq_exit()
 a lot more code runs, including things like updating jiffies when
 interrupts arrive and invoking softirq? Do we only call irq_exit() on
 the IPI path otherwise?

 Are there any plans to send this back to stable trees? Not calling
 irq_enter()/irq_exit() when we get an interrupt seems like a big problem.


Hmm I see we still call handle_IRQ eventually. So it's not as bad as I
first thought.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv12 06/49] clk: add support for low level register ops

[Stephen Boyd]

On 01/03/14 01:13, Tero Kristo wrote:
 On 12/22/2013 07:39 PM, Gerhard Sittig wrote:


 Further I'd suggest to split this register access aspect out of
 the TI clock series, and to prepare it already for regmap style
 access to the hardware registers.  See the next comment below.

 This sounds like a good idea to me, seeing it is blocking lots of
 other things.

This ll_ops struct looks like a simplified regmap. Have you seen my
series that adds regmap support to the common clock framework[1]? Is
there any reason why you can't use those patches and layer some patches
on top to add support for regmap to the basic clock types?

[1] https://lkml.org/lkml/2013/12/23/461

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: drop explicit selection of HAVE_CLK and CLKDEV_LOOKUP

[Stephen Boyd]

On 09/24, Uwe Kleine-K??nig wrote:
 CLKDEV_LOOKUP selects HAVE_CLK and COMMON_CLK selects CLKDEV_LOOKUP. So
 all symbols that select at least two of these symbols can be simplified.
 
 For imx, omap2 and ux500 some rearrangements were necessary before the
 simplification.
 
 Signed-off-by: Uwe Kleine-K??nig u.kleine-koe...@pengutronix.de

For MSM:

Acked-by: Stephen Boyd sb...@codeaurora.org

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] usb: dwc3: msm: Add device tree binding information

[Stephen Boyd]

On 08/19/13 05:27, Ivan T. Ivanov wrote:
 Hi, 

 On Fri, 2013-08-16 at 16:44 -0600, Stephen Warren wrote: 
 On 08/14/2013 06:59 AM, Ivan T. Ivanov wrote:
 From: Ivan T. Ivanov iiva...@mm-sol.com

 MSM USB3.0 core wrapper consist of USB3.0 IP from Synopsys
 (SNPS) and HS, SS PHY's control and configuration registers.

 It could operate in device mode (SS, HS, FS) and host
 mode (SS, HS, FS, LS).
 diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt 
 b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
 +- clock-names :
 ...
 +   sleep_a_clk : Sleep clock, used when USB3 core goes into low
 ...
 +   ref_clk : Reference clock - used in host mode.
 ...
 +   core_clk : Master/Core clock, have to be = 125 MHz for SS
 ...
 +   iface_clk : System bus AXI clock
 +   sleep_clk : Sleep clock, used when USB3 core goes into low
 ...
 +   utmi_clk : Generated by HS-PHY. Used to clock the low power
 I think it makes sense to remove _clk from all those names, unless the
 HW documentation really talks about a clock named e.g. iface_clk yet
 some other clock names in the documentation don't have the _clk
 suffix, e.g. the xo I didn't quote.
 From limited information that I have, I could not say how clock inputs 
 are named from the controller perspective, but I agree that _clk
 suffix looks redundant. 

In downstream trees we've tried to standardize the names on core_clk,
iface_clk, bus_clk, etc. Historically the hardware designers have used
the names from the clock controller instead of coming up with standard
names of their own when they put the clock inputs in their data sheets
(if they do at all).

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] usb: dwc3: Add Qualcomm DWC3 glue layer driver

[Stephen Boyd]

On 08/14/13 05:59, Ivan T. Ivanov wrote:
 +}
 +
 +static const struct of_device_id of_dwc3_matach[] = {

match? Maybe you can make it all one line too { .compatible = qcom,dwc3 }

 + {
 + .compatible = qcom,dwc3,
 + },
 + { },
 +};
 +MODULE_DEVICE_TABLE(of, of_dwc3_matach);
 +

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [FAILURE] omap4430-sdp allnoconfig

[Stephen Boyd]

On 08/01, Olof Johansson wrote:
 On Thu, Aug 01, 2013 at 11:59:56AM -0700, Stephen Boyd wrote:
  On 08/01, Santosh Shilimkar wrote:
   This one looks fine for me. Can you send above in
   a proper patch ? Let me know if you want me to
   send it.
   
  
  Here's a proper patch. Olof, can you please apply this on top of
  the merge (and maybe you can fix the merge again to handle the
  HAVE_CAN problem I mentioned earlier).
  
  ---8---
  Subject: [PATCH] ARM: OMAP2+: Fix fallout from localtimer divorce and SMP=n
  
  A recent patch ef3160c (ARM: OMAP2+: Divorce from local timer
  API, 2013-03-04) broke the omap build when SMP=n because the TWD
  functions are only compiled on SMP=y builds. Stub out the TWD
  calls when the TWD isn't built in to to keep everything building.
  
  arch/arm/mach-omap2/built-in.o: In function
  `omap4_local_timer_init':
  dss-common.c:(.init.text+0x1d90): undefined reference to
  `twd_local_timer_register'
  
  Reported-by: Russell King - ARM Linux li...@arm.linux.org.uk
  Cc: Santosh Shilimkar santosh.shilim...@ti.com
  Signed-off-by: Stephen Boyd sb...@codeaurora.org
 
 Thanks, applied (with the reported-by changed as requested)
 

It looks like somehow you became the author. Is there anyway you
can fix that and possibly fix the merge before it to not add
HAVE_CAN_FLEXCAN if CAN in mach-imx/Kconfig?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [FAILURE] omap4430-sdp allnoconfig

[Stephen Boyd]

On 08/02, Olof Johansson wrote:
 On Fri, Aug 2, 2013 at 1:44 PM, Stephen Boyd sb...@codeaurora.org wrote:
  On 08/01, Olof Johansson wrote:
  On Thu, Aug 01, 2013 at 11:59:56AM -0700, Stephen Boyd wrote:
   On 08/01, Santosh Shilimkar wrote:
This one looks fine for me. Can you send above in
a proper patch ? Let me know if you want me to
send it.
   
  
   Here's a proper patch. Olof, can you please apply this on top of
   the merge (and maybe you can fix the merge again to handle the
   HAVE_CAN problem I mentioned earlier).
  
   ---8---
   Subject: [PATCH] ARM: OMAP2+: Fix fallout from localtimer divorce and 
   SMP=n
  
   A recent patch ef3160c (ARM: OMAP2+: Divorce from local timer
   API, 2013-03-04) broke the omap build when SMP=n because the TWD
   functions are only compiled on SMP=y builds. Stub out the TWD
   calls when the TWD isn't built in to to keep everything building.
  
   arch/arm/mach-omap2/built-in.o: In function
   `omap4_local_timer_init':
   dss-common.c:(.init.text+0x1d90): undefined reference to
   `twd_local_timer_register'
  
   Reported-by: Russell King - ARM Linux li...@arm.linux.org.uk
   Cc: Santosh Shilimkar santosh.shilim...@ti.com
   Signed-off-by: Stephen Boyd sb...@codeaurora.org
 
  Thanks, applied (with the reported-by changed as requested)
 
 
  It looks like somehow you became the author. Is there anyway you
  can fix that and possibly fix the merge before it to not add
  HAVE_CAN_FLEXCAN if CAN in mach-imx/Kconfig?
 
 That's because the patch wasn't sent such that it could be applied
 with git am, and forgot to set authorship back. Fixed now.
 

Ah sorry, I thought you would use git am --scissors so it would
keep the authorship from the sender. I'll just add From: in
future scissored patches to make this simpler.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [FAILURE] omap4430-sdp allnoconfig

[Stephen Boyd]

On 08/01/13 11:31, Santosh Shilimkar wrote:
 On Thursday 01 August 2013 02:27 PM, Russell King - ARM Linux wrote:
 On Thu, Aug 01, 2013 at 02:11:18PM -0400, Santosh Shilimkar wrote:
 On Thursday 01 August 2013 01:52 PM, Russell King - ARM Linux wrote:
 My allnoconfig fails with this error:

 arch/arm/mach-omap2/built-in.o: In function `omap4_local_timer_init':
 dss-common.c:(.init.text+0x1d90): undefined reference to 
 `twd_local_timer_register'

 Might be worth looking into whatever's missing?
 Looks like coming from below snippet but am just wondering how ?
 Well, looking at the config allnoconfig generated, it doesn't have SMP
 and therefore it doesn't have local timers.

 The build tree has this in it:

 #ifdef CONFIG_ARCH_OMAP4
 static DEFINE_TWD_LOCAL_TIMER(twd_local_timer, OMAP44XX_LOCAL_TWD_BASE, 29);
 void __init omap4_local_timer_init(void)
 {
 omap4_sync32k_timer_init();
 /* Local timers are not supprted on OMAP4430 ES1.0 */
 if (omap_rev() != OMAP4430_REV_ES1_0) {
 int err;

 if (of_have_populated_dt()) {
 clocksource_of_init();
 return;
 }

 err = twd_local_timer_register(twd_local_timer);
 if (err)
 pr_err(twd_local_timer_register failed %d\n, err);
 }
 }
 #endif /* CONFIG_ARCH_OMAP4 */

 Now it make sense.

 which is changed from your version thanks to this commit:

 commit ef3160cd2f0a400751f2cf6fd2811225fee1d5a7
 Author: Stephen Boyd sb...@codeaurora.org
 Date:   Mon Mar 4 19:24:35 2013 -0800

 ARM: OMAP2+: Divorce from local timer API
 
 Now that the TWD doesn't rely on the local timer API, OMAP can
 stop selecting it in Kconfig and relying on the config option to
 decide if it should call smp_twd functions.
 
 Acked-by: Santosh Shilimkar santosh.shilim...@ti.com
 Acked-by: Tony Lindgren t...@atomide.com
 Acked-by: Marc Zyngier marc.zyng...@arm.com
 Signed-off-by: Stephen Boyd sb...@codeaurora.org

 So it seems LOCAL_TIMER is still actually required...

 yep. Lets see what Stephen has to say.

Hmm.. Looks like you can either wrap this up in a CONFIG_HAVE_ARM_TWD
check or just compile in TWD all the time on omap4. The latter is
simpler but not a direct conversion.

---8

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 4e0049a..c9e9b2c 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -90,7 +90,7 @@ config ARCH_OMAP4
select CACHE_L2X0
select CPU_V7
select HAVE_ARM_SCU if SMP
-   select HAVE_ARM_TWD if SMP
+   select HAVE_ARM_TWD
select HAVE_SMP
select OMAP_INTERCONNECT
select PL310_ERRATA_588369

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [FAILURE] omap4430-sdp allnoconfig

[Stephen Boyd]

On 08/01/13 11:34, Stephen Boyd wrote:
 On 08/01/13 11:31, Santosh Shilimkar wrote:
 On Thursday 01 August 2013 02:27 PM, Russell King - ARM Linux wrote:
 On Thu, Aug 01, 2013 at 02:11:18PM -0400, Santosh Shilimkar wrote:
 On Thursday 01 August 2013 01:52 PM, Russell King - ARM Linux wrote:
 My allnoconfig fails with this error:

 arch/arm/mach-omap2/built-in.o: In function `omap4_local_timer_init':
 dss-common.c:(.init.text+0x1d90): undefined reference to 
 `twd_local_timer_register'

 Might be worth looking into whatever's missing?
 Looks like coming from below snippet but am just wondering how ?
 Well, looking at the config allnoconfig generated, it doesn't have SMP
 and therefore it doesn't have local timers.

 The build tree has this in it:

 #ifdef CONFIG_ARCH_OMAP4
 static DEFINE_TWD_LOCAL_TIMER(twd_local_timer, OMAP44XX_LOCAL_TWD_BASE, 29);
 void __init omap4_local_timer_init(void)
 {
 omap4_sync32k_timer_init();
 /* Local timers are not supprted on OMAP4430 ES1.0 */
 if (omap_rev() != OMAP4430_REV_ES1_0) {
 int err;

 if (of_have_populated_dt()) {
 clocksource_of_init();
 return;
 }

 err = twd_local_timer_register(twd_local_timer);
 if (err)
 pr_err(twd_local_timer_register failed %d\n, err);
 }
 }
 #endif /* CONFIG_ARCH_OMAP4 */

 Now it make sense.

 which is changed from your version thanks to this commit:

 commit ef3160cd2f0a400751f2cf6fd2811225fee1d5a7
 Author: Stephen Boyd sb...@codeaurora.org
 Date:   Mon Mar 4 19:24:35 2013 -0800

 ARM: OMAP2+: Divorce from local timer API
 
 Now that the TWD doesn't rely on the local timer API, OMAP can
 stop selecting it in Kconfig and relying on the config option to
 decide if it should call smp_twd functions.
 
 Acked-by: Santosh Shilimkar santosh.shilim...@ti.com
 Acked-by: Tony Lindgren t...@atomide.com
 Acked-by: Marc Zyngier marc.zyng...@arm.com
 Signed-off-by: Stephen Boyd sb...@codeaurora.org

 So it seems LOCAL_TIMER is still actually required...

 yep. Lets see what Stephen has to say.
 Hmm.. Looks like you can either wrap this up in a CONFIG_HAVE_ARM_TWD
 check or just compile in TWD all the time on omap4. The latter is
 simpler but not a direct conversion.

Here's the other version (probably whitespace damaged):

---8

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 3b7caba..00dc53e 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -600,6 +600,7 @@ static OMAP_SYS_32K_TIMER_INIT(4, 1, timer_32k_ck, 
ti,timer-alwon,
 #endif
 
 #ifdef CONFIG_ARCH_OMAP4
+#ifdef CONFIG_HAVE_ARM_TWD
 static DEFINE_TWD_LOCAL_TIMER(twd_local_timer, OMAP44XX_LOCAL_TWD_BASE, 29);
 void __init omap4_local_timer_init(void)
 {
@@ -618,6 +619,12 @@ void __init omap4_local_timer_init(void)
pr_err(twd_local_timer_register failed %d\n, err);
}
 }
+#else
+void __init omap4_local_timer_init(void)
+{
+   omap4_sync32k_timer_init();
+}
+#endif /* CONFIG_HAVE_ARM_TWD */
 #endif /* CONFIG_ARCH_OMAP4 */
 
 #ifdef CONFIG_SOC_OMAP5


 ---8

 diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
 index 4e0049a..c9e9b2c 100644
 --- a/arch/arm/mach-omap2/Kconfig
 +++ b/arch/arm/mach-omap2/Kconfig
 @@ -90,7 +90,7 @@ config ARCH_OMAP4
 select CACHE_L2X0
 select CPU_V7
 select HAVE_ARM_SCU if SMP
 -   select HAVE_ARM_TWD if SMP
 +   select HAVE_ARM_TWD
 select HAVE_SMP
 select OMAP_INTERCONNECT
 select PL310_ERRATA_588369



-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [FAILURE] omap4430-sdp allnoconfig

[Stephen Boyd]

On 08/01, Santosh Shilimkar wrote:
 This one looks fine for me. Can you send above in
 a proper patch ? Let me know if you want me to
 send it.
 

Here's a proper patch. Olof, can you please apply this on top of
the merge (and maybe you can fix the merge again to handle the
HAVE_CAN problem I mentioned earlier).

---8---
Subject: [PATCH] ARM: OMAP2+: Fix fallout from localtimer divorce and SMP=n

A recent patch ef3160c (ARM: OMAP2+: Divorce from local timer
API, 2013-03-04) broke the omap build when SMP=n because the TWD
functions are only compiled on SMP=y builds. Stub out the TWD
calls when the TWD isn't built in to to keep everything building.

arch/arm/mach-omap2/built-in.o: In function
`omap4_local_timer_init':
dss-common.c:(.init.text+0x1d90): undefined reference to
`twd_local_timer_register'

Reported-by: Russell King - ARM Linux li...@arm.linux.org.uk
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Signed-off-by: Stephen Boyd sb...@codeaurora.org
---
 arch/arm/mach-omap2/timer.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 3b7caba..00dc53e 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -600,6 +600,7 @@ static OMAP_SYS_32K_TIMER_INIT(4, 1, timer_32k_ck, 
ti,timer-alwon,
 #endif
 
 #ifdef CONFIG_ARCH_OMAP4
+#ifdef CONFIG_HAVE_ARM_TWD
 static DEFINE_TWD_LOCAL_TIMER(twd_local_timer, OMAP44XX_LOCAL_TWD_BASE, 29);
 void __init omap4_local_timer_init(void)
 {
@@ -618,6 +619,12 @@ void __init omap4_local_timer_init(void)
pr_err(twd_local_timer_register failed %d\n, err);
}
 }
+#else
+void __init omap4_local_timer_init(void)
+{
+   omap4_sync32k_timer_init();
+}
+#endif /* CONFIG_HAVE_ARM_TWD */
 #endif /* CONFIG_ARCH_OMAP4 */
 
 #ifdef CONFIG_SOC_OMAP5
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 10/11] ARM: dts: omap4 clock data

[Stephen Boyd]

On 06/19/13 06:19, Tero Kristo wrote:
 diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
 index 2a56428..70608db 100644
 --- a/arch/arm/boot/dts/omap4.dtsi
 +++ b/arch/arm/boot/dts/omap4.dtsi
 @@ -106,6 +106,8 @@
   ti,hwmods = counter_32k;
   };
  
 + /include/ omap4-clocks.dtsi
 +

Doesn't this cause one platform device to be allocated for each clock
node defined in omap4-clocks.dtsi? Are you concerned about wasting
memory on things that aren't really devices and that will never be probed?

   omap4_pmx_core: pinmux@4a100040 {
   compatible = ti,omap4-padconf, pinctrl-single;
   reg = 0x4a100040 0x0196;

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: smp: Allow real broadcast device selection instead of always dummy

[Stephen Boyd]

On 03/14/13 03:28, Mark Rutland wrote:
 On Thu, Mar 14, 2013 at 07:45:14AM +, Santosh Shilimkar wrote:
 On Wednesday 13 March 2013 09:48 PM, Mark Rutland wrote:
 On Wed, Mar 13, 2013 at 03:44:03PM +, Santosh Shilimkar wrote:
 On Wednesday 13 March 2013 05:55 PM, Mark Rutland wrote:
 I do agree it'd be worth lowering the dummy timer's rating to ensure it 
 doesn't
 override a real timer elsewhere. 

 Yep. Can I add you acked-by tag then for $subject patch ?
 Would be good to get this one merged as well.
 Sure. Though could you reword the commit message? The patch solves the more
 general issue of a dummy being preferred over real hardware even outside of
 choosing the broadcast device.

 Acked-by: Mark Rutland mark.rutl...@arm.com
 Thanks. For record, patch is in end of the email which I plan
 to put into patch system.

 Regards,
 Santosh
 The below patch seems fine. Are you intending for this to go in as a fix for
 3.9-rc*, or as a cleanup for 3.10?

 If you're aiming for the latter, it's going to clash with Stephen Boyd's local
 timer API removal [1], in which the generic dummy timer driver [2] (replacing
 the arm-specific dummy [3]) also has a rating of 100.

Thanks for the heads up. Looks like the conflict will be trivial, but I
wonder why need to put the patch at all? Per my understanding the
regression has been fixed by your patch in Thomas' tree and then in 3.10
we can merge the local timer removal patches and fix up the rating at
the same time.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] clk: divider: prepare for minimum divider

[Stephen Boyd]

On 01/22/13 08:39, Afzal Mohammed wrote:
 diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
 index a9204c6..0b34992 100644
 --- a/drivers/clk/clk-divider.c
 +++ b/drivers/clk/clk-divider.c
 @@ -236,7 +236,7 @@ EXPORT_SYMBOL_GPL(clk_divider_ops);
  
  static struct clk *_register_divider(struct device *dev, const char *name,
   const char *parent_name, unsigned long flags,
 - void __iomem *reg, u8 shift, u8 width,
 + void __iomem *reg, u8 shift, u8 width, u8 min_div,
   u8 clk_divider_flags, const struct clk_div_table *table,
   spinlock_t *lock)
  {
 @@ -261,6 +261,7 @@ static struct clk *_register_divider(struct device *dev, 
 const char *name,
   div-reg = reg;
   div-shift = shift;
   div-width = width;
 + div-min_div = min_div;
   div-flags = clk_divider_flags;
   div-lock = lock;
   div-hw.init = init;
 @@ -284,16 +285,17 @@ static struct clk *_register_divider(struct device 
 *dev, const char *name,
   * @reg: register address to adjust divider
   * @shift: number of bits to shift the bitfield
   * @width: width of the bitfield
 + * @min_div: minimum allowable divider value
   * @clk_divider_flags: divider-specific flags for this clock
   * @lock: shared register lock for this clock
   */
  struct clk *clk_register_divider(struct device *dev, const char *name,
   const char *parent_name, unsigned long flags,
 - void __iomem *reg, u8 shift, u8 width,
 + void __iomem *reg, u8 shift, u8 width, u8 min_div,
   u8 clk_divider_flags, spinlock_t *lock)
  {
   return _register_divider(dev, name, parent_name, flags, reg, shift,
 - width, clk_divider_flags, NULL, lock);
 + width, min_div, clk_divider_flags, NULL, lock);
  }

Can you make a new function, clk_register_min_divider(), to avoid
disturbing all users of clock dividers that don't have a minimum? Then
the default can be put into the two registration functions in
clk-divider.c and you don't have to change all clock divider users
across the tree.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG: spinlock bad magic on CPU#0 on BeagleBone

[Stephen Boyd]

On 12/19/12 08:53, Paul Walmsley wrote:
 On Wed, 19 Dec 2012, Bedia, Vaibhav wrote:

 Current mainline on Beaglebone using the omap2plus_defconfig + 3 build fixes
 is triggering a BUG()
 ...

 [0.109688] Security Framework initialized
 [0.109889] Mount-cache hash table entries: 512
 [0.112674] BUG: spinlock bad magic on CPU#0, swapper/0/0
 [0.112724]  lock: atomic64_lock+0x240/0x400, .magic: , .owner: 
 none/-1, .owner_cpu: 0
 [0.112782] [c001af64] (unwind_backtrace+0x0/0xf0) from [c02c2010] 
 (do_raw_spin_lock+0x158/0x198)
 [0.112813] [c02c2010] (do_raw_spin_lock+0x158/0x198) from [c04d89ec] 
 (_raw_spin_lock_irqsave+0x4c/0x58)
 [0.112844] [c04d89ec] (_raw_spin_lock_irqsave+0x4c/0x58) from 
 [c02cabf0] (atomic64_add_return+0x30/0x5c)
 [0.112886] [c02cabf0] (atomic64_add_return+0x30/0x5c) from 
 [c0124564] (alloc_mnt_ns.clone.14+0x44/0xac)
 [0.112914] [c0124564] (alloc_mnt_ns.clone.14+0x44/0xac) from 
 [c0124f4c] (create_mnt_ns+0xc/0x54)
 [0.112951] [c0124f4c] (create_mnt_ns+0xc/0x54) from [c06f31a4] 
 (mnt_init+0x120/0x1d4)
 [0.112978] [c06f31a4] (mnt_init+0x120/0x1d4) from [c06f2d50] 
 (vfs_caches_init+0xe0/0x10c)
 [0.113005] [c06f2d50] (vfs_caches_init+0xe0/0x10c) from [c06d4798] 
 (start_kernel+0x29c/0x300)
 [0.113029] [c06d4798] (start_kernel+0x29c/0x300) from [80008078] 
 (0x80008078)
 [0.118290] CPU: Testing write buffer coherency: ok
 [0.118968] CPU0: thread -1, cpu 0, socket -1, mpidr 0
 [0.119053] Setting up static identity map for 0x804de2c8 - 0x804de338
 [0.120698] Brought up 1 CPUs
 This is probably a memory corruption bug, there's probably some code 
 executing early that's writing outside its own data and trashing some 
 previously-allocated memory.

I'm not so sure. It looks like atomic64s use spinlocks on processors
that don't have 64-bit atomic instructions (see lib/atomic64.c). And
those spinlocks are not initialized until a pure initcall runs,
init_atomic64_lock(). Pure initcalls don't run until after
vfs_caches_init() and so you get this BUG() warning that the spinlock is
not initialized.

How about we initialize the locks statically? Does that fix your problem?

8-

diff --git a/lib/atomic64.c b/lib/atomic64.c
index 9785378..08a4f06 100644
--- a/lib/atomic64.c
+++ b/lib/atomic64.c
@@ -31,7 +31,11 @@
 static union {
raw_spinlock_t lock;
char pad[L1_CACHE_BYTES];
-} atomic64_lock[NR_LOCKS] __cacheline_aligned_in_smp;
+} atomic64_lock[NR_LOCKS] __cacheline_aligned_in_smp = {
+   [0 ... (NR_LOCKS - 1)] = {
+   .lock =  __RAW_SPIN_LOCK_UNLOCKED(atomic64_lock.lock),
+   },
+};
 
 static inline raw_spinlock_t *lock_addr(const atomic64_t *v)
 {
@@ -173,14 +177,3 @@ int atomic64_add_unless(atomic64_t *v, long long a, long 
long u)
return ret;
 }
 EXPORT_SYMBOL(atomic64_add_unless);
-
-static int init_atomic64_lock(void)
-{
-   int i;
-
-   for (i = 0; i  NR_LOCKS; ++i)
-   raw_spin_lock_init(atomic64_lock[i].lock);
-   return 0;
-}
-
-pure_initcall(init_atomic64_lock);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG: spinlock bad magic on CPU#0 on BeagleBone

[Stephen Boyd]

On 12/19/2012 8:48 PM, Bedia, Vaibhav wrote:
 I tried out 3 variants of AM335x boards - 2 of these (BeagleBone and EVM) 
 have DDR2
 and 1 has DDR3 (EVM-SK). The BUG is triggered on all of these at the same 
 point.

 With Stephen's change I don't see this on any of the board variants :)
 New bootlog below.

Great! Can I have your Tested-by then? I'll wrap it up into a patch. Is
this is a new regression? From a glance at the code it looks to have
existed for quite a while now.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG: spinlock bad magic on CPU#0 on BeagleBone

[Stephen Boyd]

On 12/19/2012 10:44 PM, Bedia, Vaibhav wrote:
 On Thu, Dec 20, 2012 at 11:55:24, Stephen Boyd wrote:
 On 12/19/2012 8:48 PM, Bedia, Vaibhav wrote:
 I tried out 3 variants of AM335x boards - 2 of these (BeagleBone and EVM) 
 have DDR2
 and 1 has DDR3 (EVM-SK). The BUG is triggered on all of these at the same 
 point.

 With Stephen's change I don't see this on any of the board variants :)
 New bootlog below.
 Great! Can I have your Tested-by then? I'll wrap it up into a patch. Is
 this is a new regression? From a glance at the code it looks to have
 existed for quite a while now.
 I went back to a branch based off 3.7-rc4 and don't see the issue there. Not 
 sure
 what is triggering this now.

 Tested-by: Vaibhav Bedia vaibhav.be...@ti.com

Thanks. I was thrown off by the author date of this patch which
introduced your problem

commit 8823c079ba7136dc1948d6f6dcb5f8022bde438e
Author: Eric W. Biederman ebied...@xmission.com
AuthorDate: Sun Mar 7 18:49:36 2010 -0800
Commit: Eric W. Biederman ebied...@xmission.com
CommitDate: Mon Nov 19 05:59:18 2012 -0800

vfs: Add setns support for the mount namespace


It seems to have a 2 year gap between commit date and author date.
Either way, it looks to be isolated to the 3.8 merge window but affects
quite a few architectures. Patch to follow shortly.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] lib: atomic64: Initialize locks statically to fix early users

[Stephen Boyd]

The atomic64 library uses a handful of static spin locks to
implement atomic 64-bit operations on architectures without
support for atomic 64-bit instructions. Unfortunately, the
spinlocks are initialized in a pure initcall and that is too late
for the vfs namespace code which wants to use atomic64 operations
before the initcall is run (introduced by 8823c07 vfs: Add setns
support for the mount namespace).

This leads to BUG messages such as:

BUG: spinlock bad magic on CPU#0, swapper/0/0
 lock: atomic64_lock+0x240/0x400, .magic: , .owner: none/-1, 
.owner_cpu: 0
[c001af64] (unwind_backtrace+0x0/0xf0) from [c02c2010] 
(do_raw_spin_lock+0x158/0x198)
[c02c2010] (do_raw_spin_lock+0x158/0x198) from [c04d89ec] 
(_raw_spin_lock_irqsave+0x4c/0x58)
[c04d89ec] (_raw_spin_lock_irqsave+0x4c/0x58) from [c02cabf0] 
(atomic64_add_return+0x30/0x5c)
[c02cabf0] (atomic64_add_return+0x30/0x5c) from [c0124564] 
(alloc_mnt_ns.clone.14+0x44/0xac)
[c0124564] (alloc_mnt_ns.clone.14+0x44/0xac) from [c0124f4c] 
(create_mnt_ns+0xc/0x54)
[c0124f4c] (create_mnt_ns+0xc/0x54) from [c06f31a4] (mnt_init+0x120/0x1d4)
[c06f31a4] (mnt_init+0x120/0x1d4) from [c06f2d50] 
(vfs_caches_init+0xe0/0x10c)
[c06f2d50] (vfs_caches_init+0xe0/0x10c) from [c06d4798] 
(start_kernel+0x29c/0x300)
[c06d4798] (start_kernel+0x29c/0x300) from [80008078] (0x80008078)

coming out early on during boot when spinlock debugging is enabled.

Fix this problem by initializing the spinlocks statically at
compile time.

Reported-by: Vaibhav Bedia vaibhav.be...@ti.com
Tested-by: Vaibhav Bedia vaibhav.be...@ti.com
Cc: Eric W. Biederman ebied...@xmission.com
Signed-off-by: Stephen Boyd sb...@codeaurora.org
---

Sorry Andrew, I couldn't find a maintainer of this file so I
am picking on you.

 lib/atomic64.c | 17 +
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/lib/atomic64.c b/lib/atomic64.c
index 9785378..08a4f06 100644
--- a/lib/atomic64.c
+++ b/lib/atomic64.c
@@ -31,7 +31,11 @@
 static union {
raw_spinlock_t lock;
char pad[L1_CACHE_BYTES];
-} atomic64_lock[NR_LOCKS] __cacheline_aligned_in_smp;
+} atomic64_lock[NR_LOCKS] __cacheline_aligned_in_smp = {
+   [0 ... (NR_LOCKS - 1)] = {
+   .lock =  __RAW_SPIN_LOCK_UNLOCKED(atomic64_lock.lock),
+   },
+};
 
 static inline raw_spinlock_t *lock_addr(const atomic64_t *v)
 {
@@ -173,14 +177,3 @@ int atomic64_add_unless(atomic64_t *v, long long a, long 
long u)
return ret;
 }
 EXPORT_SYMBOL(atomic64_add_unless);
-
-static int init_atomic64_lock(void)
-{
-   int i;
-
-   for (i = 0; i  NR_LOCKS; ++i)
-   raw_spin_lock_init(atomic64_lock[i].lock);
-   return 0;
-}
-
-pure_initcall(init_atomic64_lock);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html