Re: [PATCH v5] ARM: imx: Add basic imx6q cpu thermal management
On Wed, Jun 20, 2012 at 02:06:04AM -0500, Robert Lee wrote:
Add imx6q cpu thermal management driver using the new cpu cooling
interface which limits system performance via cpufreq to reduce
the cpu temperature. Temperature readings are taken using
the imx6q temperature sensor and this functionality was added
as part of this cpu thermal management driver.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/boot/dts/imx6q.dtsi|5 +
drivers/thermal/Kconfig | 28 ++
drivers/thermal/Makefile|1 +
drivers/thermal/imx6q_thermal.c | 593
+++
4 files changed, 627 insertions(+)
create mode 100644 drivers/thermal/imx6q_thermal.c
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 8c90cba..2650f65 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -442,6 +442,10 @@
anatop-min-voltage = 725000;
anatop-max-voltage = 145;
};
+
+ thermal {
+ compatible =fsl,anatop-thermal;
+ };
};
usbphy@020c9000 { /* USBPHY1 */
@@ -659,6 +663,7 @@
};
ocotp@021bc000 {
+ compatible = fsl,imx6q-ocotp;
reg = 0x021bc000 0x4000;
};
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 04c6796..b80c408 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -30,6 +30,34 @@ config CPU_THERMAL
and not the ACPI interface.
If you want this support, you should say Y or M here.
+config IMX6Q_THERMAL
+ bool IMX6Q Thermal interface support
+ depends on MFD_ANATOP CPU_THERMAL
+ help
+ Adds thermal management for IMX6Q.
+
+config IMX6Q_THERMAL_FAKE_CALIBRATION
+ bool IMX6Q fake temperature sensor calibration (FOR TESTING ONLY)
+ depends on IMX6Q_THERMAL
+ help
+ This enables a fake temp sensor calibration value for parts without
+ the correction calibration values burned into OCOTP. If the part
+ already has the calibrated values burned into OCOTP, enabling this
+ does nothing.
+ WARNING: Use of this feature is for testing only as it will cause
+ incorrect temperature readings which will result in incorrect system
+ thermal limiting behavior such as premature system performance
+ limiting or lack of proper performance reduction to reduce cpu
+ temperature
+
+config IMX6Q_THERMAL_FAKE_CAL_VAL
+ hex IMX6Q fake temperature sensor calibration value
+ depends on IMX6Q_THERMAL_FAKE_CALIBRATION
+ default 0x5704c67d
+ help
+ Refer to the temperature sensor section of the imx6q reference manual
+ for more inforation on how this value is used.
Don't add such stuff to Kconfig. If during runtime you detect that there
is no calibration data, then issue a warning and fall back to a safe
value. If you really think this should be configurable, add a sysfs
entry for it. FOR TESTING ONLY seems to imply though that it shouldn't
be configurable.
+
config SPEAR_THERMAL
bool SPEAr thermal sensor driver
depends on THERMAL
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 4636e35..fc4004e 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_THERMAL) += thermal_sys.o
obj-$(CONFIG_CPU_THERMAL) += cpu_cooling.o
obj-$(CONFIG_SPEAR_THERMAL) += spear_thermal.o
obj-$(CONFIG_EXYNOS_THERMAL) += exynos_thermal.o
+obj-$(CONFIG_IMX6Q_THERMAL) += imx6q_thermal.o
diff --git a/drivers/thermal/imx6q_thermal.c b/drivers/thermal/imx6q_thermal.c
new file mode 100644
index 000..255d646
--- /dev/null
+++ b/drivers/thermal/imx6q_thermal.c
@@ -0,0 +1,593 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/* i.MX6Q Thermal Implementation */
+
+#include linux/kernel.h
+#include linux/device.h
+#include linux/module.h
+#include linux/dmi.h
+#include linux/init.h
+#include linux/slab.h
+#include linux/delay.h
+#include linux/types.h
+#include linux/thermal.h
+#include linux/io.h
+#include linux/syscalls.h
+#include linux/cpufreq.h
+#include linux/of.h
+#include linux/of_address.h
+#include linux/smp.h
+#include linux/cpu_cooling.h
+#include linux/platform_device.h
Re: [PATCH v5 0/7] cleanup imx5 idle, add imx5/6 cpuidle
Hi Robert, On Mon, May 21, 2012 at 05:50:23PM -0500, Robert Lee wrote: Cleanup up imx5 idle code and enable imx5 low powe idle for imx53. Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q platform cpuidle implementation. I rebased this to 3.5-rc1 here: git.pengutronix.de/git/imx/linux-2.6.git imx/cpuidle Could you check if the result is ok with you? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5] ARM: imx: Add basic imx6q cpu thermal management
On Wed, Jun 20, 2012 at 09:12:51AM -0500, Rob Lee wrote: Sascha, thanks for the review. + +static struct imx6q_thermal_zone *th_zone; +static void __iomem *ocotp_base; This is a driver and drivers should generally be multi instance safe. I don't understand what this comment is referring to. Could you elaborate? Drivers can only be multi instance safe when all variables are inside a instance specific struct and you pass a pointer to this struct around. What if the i.MX7 has two different ocotp_base and you want to use this driver on both ocotp? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 0/7] cleanup imx5 idle, add imx5/6 cpuidle
Hi Robert, On Mon, May 21, 2012 at 05:50:23PM -0500, Robert Lee wrote: Cleanup up imx5 idle code and enable imx5 low powe idle for imx53. Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q platform cpuidle implementation. The series looks good now. We can take this right after the merge window. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 4/7] ARM: imx: Enable imx53 low power idle
On Thu, May 17, 2012 at 09:46:21AM -0500, Rob Lee wrote:
On Wed, May 16, 2012 at 12:47 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
+void __init imx5_init_late(void)
+{
+ imx5_pm_init();
+}
+
void __init imx51_init_late(void)
{
mx51_neon_fixup();
- imx5_pm_init();
+ imx5_init_late();
}
Where would you add i.MX53 specific code above? Hint: imx5_init_late is
the wrong function name.
I added imx5_init_late for late_init functionality that is common
among all imx5. For example, in the future imx50 may use it as well.
But I can remove this and repeat the imx5_pm_init() calls for each
platform if you prefer that.
The point is that the init_late callback should have a imx53_* name,
otherwise if you call it imx5_* there is no place to add imx53 only
functionality. You can always call imx5 specific things from imx53
context, but not the other way round.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 3/7] ARM: imx: clean and consolidate imx5 suspend and idle code
On Thu, May 17, 2012 at 09:34:26AM -0500, Rob Lee wrote: On Wed, May 16, 2012 at 12:38 PM, Sascha Hauer s.ha...@pengutronix.de wrote: Hi Robert, Overall this looks ok now, some comments inline. + return ret; + + if (cpu_is_mx51()) + suspend_set_ops(mx5_suspend_ops); Now this is called only in i.MX51 context, so you can skip the cpu_is After this patch series this is also called in i.MX53 context. I'm not confident about the i.MX53 suspend/resume and would prefer to perform further testing and address issues with it in a separate patch series. In this case there is also the possibility that you add a imx51_pm_init in which you set the suspend_ops and call imx5_pm_init afterwards. The cpu_is_macros a kind of deprecated. Instead we settled on calling functions with the exact SoC name from the (dt-) board files and call the more generic function from the SoC specific ones. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 3/7] ARM: imx: clean and consolidate imx5 suspend and idle code
Hi Robert,
Overall this looks ok now, some comments inline.
Sascha
On Tue, May 15, 2012 at 09:33:32PM -0500, Robert Lee wrote:
The imx5 idle code that existed in mm-imx5.c is moved to pm-imx5.c.
The imx5_pm_init call is now exported and called during the
MACHINE_START late_init in supported imx5 platforms.
Remove various enabling/disabling of the gpc_dvfs clock and
enable it once during initialization. This is a very low
power clock that must be enabled during low power operations.
There are only two suspend_state_t imx5 low power modes ever
used. STOP_POWER_OFF for suspend to mem and
WAIT_UNCLOCKED_POWER_OFF for idle and suspend to standby. The
latter mode only requires 500 nanoseconds of extra hardware
exit time beyond a basic WFI operation (WAIT_CLOCKED mode) so
no other idle mode is necessary. Given this information, it
is more efficient to keep the registers in the often used
WAIT_UNCLOCKED_POWER_OFF state and only to and from the
STOP_POWER_OFF register state as needed when suspend to
mem is required.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-imx/mm-imx5.c | 20 +-
arch/arm/mach-imx/pm-imx5.c | 63
++-
arch/arm/plat-mxc/include/mach/common.h |3 +-
3 files changed, 40 insertions(+), 46 deletions(-)
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index d6b7e9f..bb38747 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -15,7 +15,6 @@
#include linux/init.h
#include linux/clk.h
-#include asm/system_misc.h
#include asm/mach/map.h
#include mach/hardware.h
@@ -23,23 +22,6 @@
#include mach/devices-common.h
#include mach/iomux-v3.h
-static struct clk *gpc_dvfs_clk;
-
-static void imx5_idle(void)
-{
- /* gpc clock is needed for SRPG */
- if (gpc_dvfs_clk == NULL) {
- gpc_dvfs_clk = clk_get(NULL, gpc_dvfs);
- if (IS_ERR(gpc_dvfs_clk))
- return;
- }
- clk_enable(gpc_dvfs_clk);
- mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
- if (!tzic_enable_wake())
- cpu_do_idle();
- clk_disable(gpc_dvfs_clk);
-}
-
/*
* Define the MX50 memory map.
*/
@@ -103,7 +85,6 @@ void __init imx51_init_early(void)
mxc_set_cpu_type(MXC_CPU_MX51);
mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
- arm_pm_idle = imx5_idle;
}
void __init imx53_init_early(void)
@@ -238,4 +219,5 @@ void __init imx53_soc_init(void)
void __init imx51_init_late(void)
{
mx51_neon_fixup();
+ imx5_pm_init();
}
diff --git a/arch/arm/mach-imx/pm-imx5.c b/arch/arm/mach-imx/pm-imx5.c
index e26a9cb..6e62d79 100644
--- a/arch/arm/mach-imx/pm-imx5.c
+++ b/arch/arm/mach-imx/pm-imx5.c
@@ -13,18 +13,27 @@
#include linux/io.h
#include linux/err.h
#include asm/cacheflush.h
+#include asm/system_misc.h
#include asm/tlbflush.h
#include mach/common.h
#include mach/hardware.h
#include crm-regs-imx5.h
-static struct clk *gpc_dvfs_clk;
+/*
+ * The WAIT_UNCLOCKED_POWER_OFF state only requires = 500ns to exit.
+ * This is also the lowest power state possible without affecting
+ * non-cpu parts of the system. For these reasons, imx5 should default
+ * to always using this state for cpu idling. The PM_SUSPEND_STANDBY also
+ * uses this state and needs to take no action when registers remain
confgiured
+ * for this state.
+ */
+#define IMX5_DEFAULT_CPU_IDLE_STATE WAIT_UNCLOCKED_POWER_OFF
/*
* set cpu low power mode before WFI instruction. This function is called
* mx5 because it can be used for mx50, mx51, and mx53.
*/
-void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
+static void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
{
u32 plat_lpc, arm_srpgcr, ccm_clpcr;
u32 empgc0, empgc1;
@@ -87,11 +96,6 @@ void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
}
}
-static int mx5_suspend_prepare(void)
-{
- return clk_prepare_enable(gpc_dvfs_clk);
-}
-
static int mx5_suspend_enter(suspend_state_t state)
{
switch (state) {
@@ -99,7 +103,7 @@ static int mx5_suspend_enter(suspend_state_t state)
mx5_cpu_lp_set(STOP_POWER_OFF);
break;
case PM_SUSPEND_STANDBY:
- mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
+ /* DEFAULT_IDLE_STATE already configured */
break;
default:
return -EINVAL;
@@ -114,12 +118,10 @@ static int mx5_suspend_enter(suspend_state_t state)
__raw_writel(0, MXC_SRPG_EMPGC1_SRPGCR);
}
cpu_do_idle();
- return 0;
-}
-static void mx5_suspend_finish(void)
-{
- clk_disable_unprepare(gpc_dvfs_clk);
+ /* return registers to default idle state */
+ mx5_cpu_lp_set(IMX5_DEFAULT_CPU_IDLE_STATE);
+ return 0;
}
static
Re: [PATCH v4 4/7] ARM: imx: Enable imx53 low power idle
On Tue, May 15, 2012 at 09:33:33PM -0500, Robert Lee wrote:
Add various functionality needed to enable a imx53 low power idle
state. This includes adding the imx53 gpc_dvfs clock and making a
common imx5_late_init function and initializing all imx53
MACHINE_STATE late_init calls to imx5_late_init.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-imx/clock-mx51-mx53.c |1 +
arch/arm/mach-imx/imx53-dt.c|1 +
arch/arm/mach-imx/mach-mx53_ard.c |1 +
arch/arm/mach-imx/mach-mx53_evk.c |1 +
arch/arm/mach-imx/mach-mx53_loco.c |1 +
arch/arm/mach-imx/mach-mx53_smd.c |1 +
arch/arm/mach-imx/mm-imx5.c |7 ++-
arch/arm/plat-mxc/include/mach/common.h |1 +
8 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-imx/clock-mx51-mx53.c
b/arch/arm/mach-imx/clock-mx51-mx53.c
index 0847050..decedc6 100644
--- a/arch/arm/mach-imx/clock-mx51-mx53.c
+++ b/arch/arm/mach-imx/clock-mx51-mx53.c
@@ -1529,6 +1529,7 @@ static struct clk_lookup mx53_lookups[] = {
_REGISTER_CLOCK(imx-ssi.1, NULL, ssi2_clk)
_REGISTER_CLOCK(imx-ssi.2, NULL, ssi3_clk)
_REGISTER_CLOCK(imx-keypad, NULL, dummy_clk)
+ _REGISTER_CLOCK(NULL, gpc_dvfs, gpc_dvfs_clk)
This has to be rebased against the common clock patches.
.timer = mx53_smd_timer,
.init_machine = mx53_smd_board_init,
+ .init_late = imx5_init_late,
.restart= mxc_restart,
MACHINE_END
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index bb38747..7740739 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -216,8 +216,13 @@ void __init imx53_soc_init(void)
ARRAY_SIZE(imx53_audmux_res));
}
+void __init imx5_init_late(void)
+{
+ imx5_pm_init();
+}
+
void __init imx51_init_late(void)
{
mx51_neon_fixup();
- imx5_pm_init();
+ imx5_init_late();
}
Where would you add i.MX53 specific code above? Hint: imx5_init_late is
the wrong function name.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 5/7] ARM: imx: Add common imx cpuidle init functionality.
On Tue, May 15, 2012 at 09:33:34PM -0500, Robert Lee wrote:
Add common cpuidle init functionality that can be used by various
imx platforms.
Signed-off-by: Robert Lee rob@linaro.org
---
+
+#ifdef CONFIG_CPU_IDLE
+extern int imx_cpuidle_init(struct cpuidle_driver *drv);
+#else
+static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
+{
+ return -ENODEV;
+}
+#endif
You should return succesfully here. Think about it, if imx_cpuidle_init
fails you basically do nothing except maybe printing an error message
which will be irritating when it appears on a kernel with cpuidle
disabled.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 2/3] ARM: imx: Add imx5 cpuidle driver
On Mon, May 07, 2012 at 04:16:46PM -0500, Robert Lee wrote:
Add imx5 cpuidle driver.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-imx/mm-imx5.c | 42 +++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index d6b7e9f..0b3a4cc 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -20,26 +20,61 @@
#include mach/hardware.h
#include mach/common.h
+#include mach/cpuidle.h
#include mach/devices-common.h
#include mach/iomux-v3.h
static struct clk *gpc_dvfs_clk;
-static void imx5_idle(void)
+static int imx5_idle(void)
{
+ int ret = 0;
+
/* gpc clock is needed for SRPG */
if (gpc_dvfs_clk == NULL) {
gpc_dvfs_clk = clk_get(NULL, gpc_dvfs);
This clk_get should go away here and be moved somewhere to
initialization. Also, if getting this clock fails we can still
do regular cpu_do_idle. Additionally, if clk_get fails, we'll
have a ERR_PTR value in gpc_dvfs_clk in which case the
gpc_dvfs_clk == NULL won't trigger next time you are here and
then you'll enable a nonexisting clock below.
if (IS_ERR(gpc_dvfs_clk))
- return;
+ return -ENODEV;
}
clk_enable(gpc_dvfs_clk);
mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
if (!tzic_enable_wake())
cpu_do_idle();
+ else
+ ret = -EBUSY;
clk_disable(gpc_dvfs_clk);
+
+ return ret;
+}
+
+static int imx5_cpuidle_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int idx)
+{
+ int ret;
+
+ ret = imx5_idle();
+
+ if (ret 0)
+ return ret;
+
+ return idx;
}
+static struct cpuidle_driver imx5_cpuidle_driver = {
+ .name = imx5_cpuidle,
+ .owner = THIS_MODULE,
+ .en_core_tk_irqen = 1,
+ .states[0] = {
+ .enter = imx5_cpuidle_enter,
+ .exit_latency = 20, /* max latency at 160MHz */
+ .target_residency = 1,
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .name = IMX5 SRPG,
+ .desc = CPU state retained,powered off,
+ },
I wonder why you don't add the default ARM_CPUIDLE_WFI_STATE_PWR state.
The above is something different, right? It has a greater exit latency
than ARM_CPUIDLE_WFI_STATE_PWR, so why don't we add it here aswell?
+ .state_count= 1,
+};
+
/*
* Define the MX50 memory map.
*/
@@ -103,7 +138,7 @@ void __init imx51_init_early(void)
mxc_set_cpu_type(MXC_CPU_MX51);
mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
- arm_pm_idle = imx5_idle;
+ arm_pm_idle = (void (*)(void))imx5_idle;
Still this looks suspicious. Reading this will lead to the question why
this prototype is casted. Please just add a imx5_pm_idle with the
correct prototype.
}
void __init imx53_init_early(void)
@@ -238,4 +273,5 @@ void __init imx53_soc_init(void)
void __init imx51_init_late(void)
{
mx51_neon_fixup();
+ imx_cpuidle_init(imx5_cpuidle_driver);
}
--
1.7.10
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 2/5] clk: prevent spurious parent rate propagation
On Sun, May 06, 2012 at 10:08:27PM -0700, Mike Turquette wrote:
Patch 'clk: always pass parent_rate into .round_rate' made a subtle
change to the semantics of .round_rate. It is now expected for the
parent's rate to always be passed in, simplifying the implemenation of
various .round_rate callback definitions.
However the patch also introduced a bug in clk_calc_new_rates whereby a
clock without the CLK_SET_RATE_PARENT flag set could still propagate a
rate change up to a parent clock if the the .round_rate callback
modified the best_parent_rate value in any way.
This patch fixes the issue at the framework level (in
clk_calc_new_rates) by specifically handling the case where the
CLK_SET_RATE_PARENT flag is not set.
Signed-off-by: Mike Turquette mturque...@linaro.org
Reported-by: Sascha Hauers.ha...@pengutronix.de
I think a warning might be useful when a clk driver tries to change the
parent rate even if it is not allowed to. This can be added in a later
patch, so
Acked-by: Sascha Hauer s.ha...@pengutronix.de
Sascha
---
drivers/clk/clk.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8149764..7ceca0e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -774,12 +774,18 @@ static struct clk *clk_calc_new_rates(struct clk *clk,
unsigned long rate)
if (IS_ERR_OR_NULL(clk))
return NULL;
+ /* save parent rate, if it exists */
+ if (clk-parent)
+ best_parent_rate = clk-parent-rate;
+
/* never propagate up to the parent */
if (!(clk-flags CLK_SET_RATE_PARENT)) {
if (!clk-ops-round_rate) {
clk-new_rate = clk-rate;
return NULL;
}
+ new_rate = clk-ops-round_rate(clk-hw, rate,
best_parent_rate);
+ goto out;
}
/* need clk-parent from here on out */
@@ -795,7 +801,6 @@ static struct clk *clk_calc_new_rates(struct clk *clk,
unsigned long rate)
goto out;
}
- best_parent_rate = clk-parent-rate;
new_rate = clk-ops-round_rate(clk-hw, rate, best_parent_rate);
if (best_parent_rate != clk-parent-rate) {
--
1.7.5.4
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Making ARM multiplatform kernels DT-only?
On Fri, May 04, 2012 at 04:24:17PM +, Arnd Bergmann wrote: On Thursday 03 May 2012, Sascha Hauer wrote: I don't think that enforcing DT only in multiplatform kernels will speed up porting to DT. As a platform maintainer I am interested in building multiplatform Kernels, but our customers are mostly uninterested in this. They probably disable other platforms anyway to save the binary space. I was not asking about enabling multiple board files but multiple mach-* directories, Yes, I understood that. which is something that I'm probably more interested in than you are, and the customers you refer to would certainly not do that if they only want to run on one board. This is really about people who distribute kernels that run on a wide variety of machines across soc vendor boundaries, people like ubuntu or cyanogenmod. The question is really whether you see a reason why they should enable the 25 non-DT board files on your platform, rather than helping out getting DT support for the machines they are interested in? They should not if they are not interested in these boards, but why shouldn't I be able to enable these 25 boards plus a few atmel or pxa boards? When there are technical reasons to limit a multiplatform Kernel to DT only, then fine, lets do it that way. If there are no technical reasons and this limitation shall only be used to put some political pressure on platform board maintainers, then I am against it. Look around, people actually *are* porting their boards over to device tree, I don't think that such pressure is necessary. Only my two cents, it's not that important to me since I want to port my (relevant) boards over to DT anyway, so I won't argue about this. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 2/3] ARM: imx: Add imx5 cpuidle driver
On Wed, May 02, 2012 at 03:11:35PM -0500, Rob Lee wrote: Sascha, mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR)); mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR)); - arm_pm_idle = imx5_idle; + arm_pm_idle = (void *)imx5_idle; I don't like this. It will cover all warnings when the prototype of arm_pm_idle changes in future. Better add a static void imx5_idle which calls a static int imx5_do_idle, then you have an idle function which returns an int. What about using the following: arm_pm_idle = (void (*)(void))imx5_idle; This will give warnings if arm_pm_idle prototype changes. This surely works but will look suspicious for people looking at the code. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
Add common cpuidle init functionality that can be used by various
imx platforms.
Signed-off-by: Robert Lee rob@linaro.org
---
+
+int __init imx_cpuidle_init(struct cpuidle_driver *drv)
+{
+ struct cpuidle_device *dev;
+ int cpu_id, ret;
+
+ if (!drv || drv-state_count CPUIDLE_STATE_MAX) {
Please don't check for !drv here. When someone calls this function with
a NULL pointer he should get a nive stack trace allowing him to figure
out what went wrong.
+ pr_err(%s: Invalid Input\n, __func__);
+ return -EINVAL;
+ }
+
+ ret = cpuidle_register_driver(drv);
+ if (ret) {
+ pr_err(%s: Failed to register cpuidle driver with error: %d\n,
+ __func__, ret);
+ return ret;
+ }
+
+ imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+ if (imx_cpuidle_devices == NULL) {
+ ret = -ENOMEM;
+ goto unregister_drv;
+ }
+
+ /* initialize state data for each cpuidle_device */
+ for_each_possible_cpu(cpu_id) {
+ dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+ dev-cpu = cpu_id;
+ dev-state_count = drv-state_count;
+
+ ret = cpuidle_register_device(dev);
+ if (ret) {
+ pr_err(%s: Failed to register cpu %u\n,
+ __func__, cpu_id);
+ goto uninit;
You should only unregister the cpuidle devices you successfully
registered. Unregistering not yet registered cpuidle devices probably
has unwanted side effects.
Sascha
+ }
+ }
+
+ return 0;
+
+uninit:
+ imx_cpuidle_devices_uninit();
+
+unregister_drv:
+ cpuidle_unregister_driver(drv);
+ return ret;
+}
diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h
b/arch/arm/plat-mxc/include/mach/cpuidle.h
new file mode 100644
index 000..8612510
--- /dev/null
+++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include linux/cpuidle.h
+
+#ifdef CONFIG_CPU_IDLE
+extern void imx_cpuidle_devices_uninit(void);
+extern int imx_cpuidle_init(struct cpuidle_driver *drv);
+#else
+static inline void imx_cpuidle_devices_uninit(void) {}
+static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
+{ return -ENODEV; }
+#endif
--
1.7.10
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 2/3] ARM: imx: Add imx5 cpuidle driver
On Tue, May 01, 2012 at 09:12:39PM -0500, Robert Lee wrote:
Add imx5 cpuidle driver.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-imx/mm-imx5.c | 42 +++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index d6b7e9f..cbd9bad 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -22,24 +22,59 @@
#include mach/common.h
#include mach/devices-common.h
#include mach/iomux-v3.h
+#include mach/cpuidle.h
static struct clk *gpc_dvfs_clk;
-static void imx5_idle(void)
+static int imx5_idle(void)
{
+ int ret = 0;
+
/* gpc clock is needed for SRPG */
if (gpc_dvfs_clk == NULL) {
gpc_dvfs_clk = clk_get(NULL, gpc_dvfs);
if (IS_ERR(gpc_dvfs_clk))
- return;
+ return -ENODEV;
}
clk_enable(gpc_dvfs_clk);
mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
if (!tzic_enable_wake())
cpu_do_idle();
+ else
+ ret = -EBUSY;
clk_disable(gpc_dvfs_clk);
+
+ return ret;
+}
+
+static int imx5_cpuidle_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int idx)
+{
+ int ret;
+
+ ret = imx5_idle();
+
+ if (ret 0)
+ return ret;
+
+ return idx;
}
+static struct cpuidle_driver imx5_cpuidle_driver = {
+ .name = imx5_cpuidle,
+ .owner = THIS_MODULE,
+ .en_core_tk_irqen = 1,
+ .states[0] = {
+ .enter = imx5_cpuidle_enter,
+ .exit_latency = 20, /* max latency at 160MHz */
+ .target_residency = 1,
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .name = IMX5 SRPG,
+ .desc = CPU state retained,powered off,
+ },
+ .state_count= 1,
+};
+
/*
* Define the MX50 memory map.
*/
@@ -103,7 +138,7 @@ void __init imx51_init_early(void)
mxc_set_cpu_type(MXC_CPU_MX51);
mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
- arm_pm_idle = imx5_idle;
+ arm_pm_idle = (void *)imx5_idle;
I don't like this. It will cover all warnings when the prototype of
arm_pm_idle changes in future. Better add a static void imx5_idle
which calls a static int imx5_do_idle, then you have an idle function
which returns an int.
}
void __init imx53_init_early(void)
@@ -238,4 +273,5 @@ void __init imx53_soc_init(void)
void __init imx51_init_late(void)
{
mx51_neon_fixup();
+ imx_cpuidle_init(imx5_cpuidle_driver);
}
--
1.7.10
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Wed, May 02, 2012 at 02:16:36PM -0500, Rob Lee wrote:
Sascha,
+int __init imx_cpuidle_init(struct cpuidle_driver *drv)
+{
+ struct cpuidle_device *dev;
+ int cpu_id, ret;
+
+ if (!drv || drv-state_count CPUIDLE_STATE_MAX) {
Please don't check for !drv here. When someone calls this function with
a NULL pointer he should get a nive stack trace allowing him to figure
out what went wrong.
Ok, I will change this in v3. Given your statement, my understanding
is that I should avoid adding checks to make sure a valid driver
object was given as the stack trace information is all the handling
that is needed. If there is any further logic needed in that rule,
could you elaborate so that I don't make this mistake in the future,
or so that I don't add a check on a driver object in a case that I
should?
Here we have the case that only a Kernel developer will add a call to
this function. For a kernel developer a stack trace is more useful
than a pr_err. Of course this is different when not testing for a NULL
pointer causes subtle bugs in unrelated code.
You should only unregister the cpuidle devices you successfully
registered. Unregistering not yet registered cpuidle devices probably
has unwanted side effects.
I did not add in this handling because the cpuidle_unregister_device()
call already has a registered check so extra handling seemed
unnecessary. But if you still think it is needed just let me know.
It's ok then. I didn't check cpuidle_unregister_device.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Mon, Apr 23, 2012 at 01:18:21PM +0800, Shawn Guo wrote:
On Sun, Apr 22, 2012 at 11:44:39PM -0500, Rob Lee wrote:
I don't think we need a cpu_is_imx6q(), but having some i.MX6 specific
hook at device_initcall time can't be too wrong. Shawn?
Yep, it works for me.
Sascha, Shawn, thanks for the response.
Since device_initcall isn't platform specific, it seems I would still
need a cpu_is_imx6q() function or similiar functionality from a device
tree call. Or do you have something else in mind that I'm not seeing?
I guess Sascha is asking for something like the following.
static int __init imx_device_init(void)
{
imx5_device_init();
imx6_device_init();
}
device_initcall(imx_device_init)
static int __init imx6_device_init(void)
{
/*
* do whatever needs to get done in device_initcall time
*/
}
The problem is more how we can detect that we are actually running a
i.MX6 SoC. We could directly ask the devicetree in an initcall or we
could introduce a cpu_is_mx6() just like we have a macro for all other
i.MX SoCs.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Mon, Apr 23, 2012 at 02:53:01PM +0800, Shawn Guo wrote:
On Mon, Apr 23, 2012 at 08:27:39AM +0200, Sascha Hauer wrote:
On Mon, Apr 23, 2012 at 01:18:21PM +0800, Shawn Guo wrote:
On Sun, Apr 22, 2012 at 11:44:39PM -0500, Rob Lee wrote:
I don't think we need a cpu_is_imx6q(), but having some i.MX6
specific
hook at device_initcall time can't be too wrong. Shawn?
Yep, it works for me.
Sascha, Shawn, thanks for the response.
Since device_initcall isn't platform specific, it seems I would still
need a cpu_is_imx6q() function or similiar functionality from a device
tree call. Or do you have something else in mind that I'm not seeing?
I guess Sascha is asking for something like the following.
static int __init imx_device_init(void)
{
imx5_device_init();
imx6_device_init();
}
device_initcall(imx_device_init)
static int __init imx6_device_init(void)
{
/*
* do whatever needs to get done in device_initcall time
*/
}
The problem is more how we can detect that we are actually running a
i.MX6 SoC. We could directly ask the devicetree in an initcall or we
could introduce a cpu_is_mx6() just like we have a macro for all other
i.MX SoCs.
Oops, my reply was a brain-dead one. Hmm, then it seems that we have
to introduce cpu_is_mx6() which I tried hard to avoid. I do not have
a preference between defining a macro and asking device tree.
Since we already have a place in early setup code in which we know that
we are running on an i.MX6 I suggest for the sake of the symmetry of the
universe that we introduce a cpu_is_mx6.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Mon, Apr 23, 2012 at 03:10:15PM +0800, Shawn Guo wrote:
On Mon, Apr 23, 2012 at 08:56:23AM +0200, Sascha Hauer wrote:
On Mon, Apr 23, 2012 at 02:53:01PM +0800, Shawn Guo wrote:
On Mon, Apr 23, 2012 at 08:27:39AM +0200, Sascha Hauer wrote:
On Mon, Apr 23, 2012 at 01:18:21PM +0800, Shawn Guo wrote:
On Sun, Apr 22, 2012 at 11:44:39PM -0500, Rob Lee wrote:
...
i.MX6 SoC. We could directly ask the devicetree in an initcall or we
could introduce a cpu_is_mx6() just like we have a macro for all other
i.MX SoCs.
Oops, my reply was a brain-dead one. Hmm, then it seems that we have
to introduce cpu_is_mx6() which I tried hard to avoid. I do not have
a preference between defining a macro and asking device tree.
Since we already have a place in early setup code in which we know that
we are running on an i.MX6 I suggest for the sake of the symmetry of the
universe that we introduce a cpu_is_mx6.
Let me try last time. What about having a late_initcall hook in
machine_desc?
Also fine with me.
Regards,
Shawn
8---
diff --git a/arch/arm/include/asm/mach/arch.h
b/arch/arm/include/asm/mach/arch.h
index d7692ca..0b1c94b 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -43,6 +43,7 @@ struct machine_desc {
void(*init_irq)(void);
struct sys_timer*timer; /* system tick timer*/
void(*init_machine)(void);
+ void(*init_late)(void);
#ifdef CONFIG_MULTI_IRQ_HANDLER
void(*handle_irq)(struct pt_regs *);
#endif
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index ebfac78..549f036 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -800,6 +800,14 @@ static int __init customize_machine(void)
}
arch_initcall(customize_machine);
+static int __init init_machine_late(void)
+{
+ if (machine_desc-init_late)
+ machine_desc-init_late();
+ return 0;
+}
+late_initcall(init_machine_late);
+
#ifdef CONFIG_KEXEC
static inline unsigned long long get_total_mem(void)
{
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index da6c1d9..0e3640f 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -142,6 +142,7 @@ DT_MACHINE_START(IMX6Q, Freescale i.MX6 Quad (Device
Tree))
.handle_irq = imx6q_handle_irq,
.timer = imx6q_timer,
.init_machine = imx6q_init_machine,
+ .init_late = imx6q_init_late,
.dt_compat = imx6q_dt_compat,
.restart= imx6q_restart,
MACHINE_END
___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Wed, Apr 18, 2012 at 11:18:55PM -0500, Rob Lee wrote: If I called imx_cpuidle_init directly from imx5 or imx6q init routines, it would be getting called before the coreinit_call of core cpuidle causing a failure. There were various other directions to take and all seemed less desirable than this one. One alternative would be to add a function to return the pointer to the cpuidle driver object based on the machine type. Functionality exists to identify imx5 as a machine type but not imx6q, so I couldn't use that machine based method without adding that extra code. Another alternative would be to add a general platform lateinit_call function to each platforms that support cpuidle. Just put the initcall into mm-imx5.c and check the cpu type. Then you also don't have to make imx5_idle global. That solution is currently available for imx5 but for imx6q it implies adding the cpu type support for imx6q. Are you ok with that? Sascha or Shawn, any further comments on my question? I don't think we need a cpu_is_imx6q(), but having some i.MX6 specific hook at device_initcall time can't be too wrong. Shawn? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: imx: Fix imx5 idle logic bug
On Tue, Apr 17, 2012 at 09:11:32AM -0500, Rob Lee wrote: On Tue, Apr 17, 2012 at 3:10 AM, Sascha Hauer s.ha...@pengutronix.de wrote: On Mon, Apr 16, 2012 at 06:37:48PM -0500, Robert Lee wrote: The imx5_idle() check of the tzic_eanble_wake() return value uses incorrect (inverted) logic causing all attempt to idle to fail. Does this have influence on current kernels or does this only trigger with your cpuidle patches? This influences non-cpuidle kernels also as imx5_idle is what arm_pm_idle points to for imx51. Ok, Applied this one for -rc then Sascha Sascha Signed-off-by: Robert Lee rob@linaro.org --- arch/arm/mach-imx/mm-imx5.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c index 05250ae..e10f391 100644 --- a/arch/arm/mach-imx/mm-imx5.c +++ b/arch/arm/mach-imx/mm-imx5.c @@ -35,7 +35,7 @@ static void imx5_idle(void) } clk_enable(gpc_dvfs_clk); mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF); - if (tzic_enable_wake() != 0) + if (!tzic_enable_wake()) cpu_do_idle(); clk_disable(gpc_dvfs_clk); } -- 1.7.10 ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Mon, Apr 16, 2012 at 06:50:12PM -0500, Robert Lee wrote:
Add common cpuidle init functionality that can be used by various
imx platforms.
Signed-off-by: Robert Lee rob@linaro.org
---
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index e81290c..7c9e05f 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -16,6 +16,8 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
obj-$(CONFIG_MXC_USE_EPIT) += epit.o
obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
obj-$(CONFIG_CPU_FREQ_IMX)+= cpufreq.o
+obj-$(CONFIG_CPU_IDLE)+= cpuidle.o
+
ifdef CONFIG_SND_IMX_SOC
obj-y += ssi-fiq.o
obj-y += ssi-fiq-ksym.o
diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
new file mode 100644
index 000..d1c9301
--- /dev/null
+++ b/arch/arm/plat-mxc/cpuidle.c
@@ -0,0 +1,89 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include linux/kernel.h
+#include linux/io.h
+#include linux/cpuidle.h
+#include linux/hrtimer.h
+#include linux/err.h
+#include linux/slab.h
+
+static struct cpuidle_device __percpu * imx_cpuidle_devices;
+static struct cpuidle_driver *drv;
+
+void __init imx_cpuidle_set_driver(struct cpuidle_driver *p)
+{
+ drv = p;
+}
You like it complicated, eh? Why do you introduce a function which sets
a variable...
+
+void imx_cpuidle_devices_uninit(void)
+{
+ int cpu_id;
+ struct cpuidle_device *dev;
+
+ for_each_possible_cpu(cpu_id) {
+ dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+ cpuidle_unregister_device(dev);
+ }
+
+ free_percpu(imx_cpuidle_devices);
+}
+
+static int __init imx_cpuidle_init(void)
... instead of passing it directly to the function which uses it?
+{
+ struct cpuidle_device *dev;
+ int cpu_id, ret;
+
+ if (!drv || drv-state_count CPUIDLE_STATE_MAX) {
+ pr_err(%s: Invalid Input\n, __func__);
You introduce a pointless error message on SoCs not setting a cpuidle
driver. When will people learn that initcalls do not only run on
machines they have on their desk?
+ return -EINVAL;
+ }
+
+ ret = cpuidle_register_driver(drv);
+
+ if (ret) {
+ pr_err(%s: Failed to register cpuidle driver\n, __func__);
It's always nice to add the return value to the error message to get a
clue *what* went wrong. The function name though is rather
uninteresting.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: imx: Fix imx5 idle logic bug
On Mon, Apr 16, 2012 at 06:37:48PM -0500, Robert Lee wrote: The imx5_idle() check of the tzic_eanble_wake() return value uses incorrect (inverted) logic causing all attempt to idle to fail. Does this have influence on current kernels or does this only trigger with your cpuidle patches? Sascha Signed-off-by: Robert Lee rob@linaro.org --- arch/arm/mach-imx/mm-imx5.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c index 05250ae..e10f391 100644 --- a/arch/arm/mach-imx/mm-imx5.c +++ b/arch/arm/mach-imx/mm-imx5.c @@ -35,7 +35,7 @@ static void imx5_idle(void) } clk_enable(gpc_dvfs_clk); mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF); - if (tzic_enable_wake() != 0) + if (!tzic_enable_wake()) cpu_do_idle(); clk_disable(gpc_dvfs_clk); } -- 1.7.10 ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Tue, Apr 17, 2012 at 08:54:03AM -0500, Rob Lee wrote:
On Tue, Apr 17, 2012 at 2:43 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Mon, Apr 16, 2012 at 06:50:12PM -0500, Robert Lee wrote:
Add common cpuidle init functionality that can be used by various
imx platforms.
Signed-off-by: Robert Lee rob@linaro.org
---
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index e81290c..7c9e05f 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -16,6 +16,8 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
obj-$(CONFIG_MXC_USE_EPIT) += epit.o
obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o
+obj-$(CONFIG_CPU_IDLE) += cpuidle.o
+
ifdef CONFIG_SND_IMX_SOC
obj-y += ssi-fiq.o
obj-y += ssi-fiq-ksym.o
diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
new file mode 100644
index 000..d1c9301
--- /dev/null
+++ b/arch/arm/plat-mxc/cpuidle.c
@@ -0,0 +1,89 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include linux/kernel.h
+#include linux/io.h
+#include linux/cpuidle.h
+#include linux/hrtimer.h
+#include linux/err.h
+#include linux/slab.h
+
+static struct cpuidle_device __percpu * imx_cpuidle_devices;
+static struct cpuidle_driver *drv;
+
+void __init imx_cpuidle_set_driver(struct cpuidle_driver *p)
+{
+ drv = p;
+}
You like it complicated, eh? Why do you introduce a function which sets
a variable...
This complication is used to deal with the timing of various levels of
init calls. More explanation below.
+
+void imx_cpuidle_devices_uninit(void)
+{
+ int cpu_id;
+ struct cpuidle_device *dev;
+
+ for_each_possible_cpu(cpu_id) {
+ dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+ cpuidle_unregister_device(dev);
+ }
+
+ free_percpu(imx_cpuidle_devices);
+}
+
+static int __init imx_cpuidle_init(void)
... instead of passing it directly to the function which uses it?
If I called imx_cpuidle_init directly from imx5 or imx6q init
routines, it would be getting called before the coreinit_call of core
cpuidle causing a failure. There were various other directions to
take and all seemed less desirable than this one.
One alternative would be to add a function to return the pointer to
the cpuidle driver object based on the machine type. Functionality
exists to identify imx5 as a machine type but not imx6q, so I couldn't
use that machine based method without adding that extra code.
Another alternative would be to add a general platform lateinit_call
function to each platforms that support cpuidle.
Just put the initcall into mm-imx5.c and check the cpu type. Then you
also don't have to make imx5_idle global.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 12/13] clk: core: copy parent_names return error codes
On Wed, Apr 11, 2012 at 06:02:50PM -0700, Mike Turquette wrote:
This patch cleans up clk_register and solves a few bugs by teaching
clk_register and __clk_init to return error codes (instead of just NULL)
to better align with the existing clk.h api.
Along with that change this patch also introduces a new behavior whereby
clk_register copies the parent_names array, thus allowing platforms to
declare their parent_names arrays as __initdata.
Signed-off-by: Mike Turquette mturque...@linaro.org
Cc: Arnd Bergman arnd.bergm...@linaro.org
Cc: Olof Johansson o...@lixom.net
Cc: Russell King li...@arm.linux.org.uk
Cc: Sascha Hauer s.ha...@pengutronix.de
Cc: Shawn Guo shawn@freescale.com
Cc: Richard Zhao richard.z...@linaro.org
Cc: Saravana Kannan skan...@codeaurora.org
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Andrew Lunn and...@lunn.ch
Cc: Rajendra Nayak rna...@ti.com
Cc: Viresh Kumar viresh.ku...@st.com
---
drivers/clk/clk.c| 61 +
include/linux/clk-private.h |4 ++-
include/linux/clk-provider.h |3 +-
3 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ddade87..af2bf12 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1185,34 +1185,41 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
* very large numbers of clocks that need to be statically initialized. It
is
* a layering violation to include clk-private.h from any code which
implements
* a clock's .ops; as such any statically initialized clock data MUST be in a
- * separate C file from the logic that implements it's operations.
+ * separate C file from the logic that implements it's operations. Returns 0
+ * on success, otherwise an error code.
*/
-void __clk_init(struct device *dev, struct clk *clk)
+int __clk_init(struct device *dev, struct clk *clk)
{
- int i;
+ int i, ret = 0;
struct clk *orphan;
struct hlist_node *tmp, *tmp2;
if (!clk)
- return;
+ return -EINVAL;
mutex_lock(prepare_lock);
/* check to see if a clock with this name is already registered */
- if (__clk_lookup(clk-name))
+ if (__clk_lookup(clk-name)) {
+ pr_debug(%s: clk %s already initialized\n,
+ __func__, clk-name);
+ ret = -EEXIST;
goto out;
+ }
/* check that clk_ops are sane. See Documentation/clk.txt */
if (clk-ops-set_rate
!(clk-ops-round_rate clk-ops-recalc_rate)) {
pr_warning(%s: %s must implement .round_rate .recalc_rate\n,
__func__, clk-name);
+ ret = -EINVAL;
goto out;
}
if (clk-ops-set_parent !clk-ops-get_parent) {
pr_warning(%s: %s must implement .get_parent .set_parent\n,
__func__, clk-name);
+ ret = -EINVAL;
goto out;
}
@@ -1308,7 +1315,7 @@ void __clk_init(struct device *dev, struct clk *clk)
out:
mutex_unlock(prepare_lock);
- return;
+ return ret;
}
/**
@@ -1324,29 +1331,59 @@ out:
* clk_register is the primary interface for populating the clock tree with
new
* clock nodes. It returns a pointer to the newly allocated struct clk which
* cannot be dereferenced by driver code but may be used in conjuction with
the
- * rest of the clock API.
+ * rest of the clock API. In the event of an error clk_register will return
an
+ * error code; drivers must test for an error code after calling
clk_register.
*/
struct clk *clk_register(struct device *dev, const char *name,
const struct clk_ops *ops, struct clk_hw *hw,
const char **parent_names, u8 num_parents, unsigned long flags)
{
+ int i, ret = -ENOMEM;
I suggest to move the initialization of ret from here...
struct clk *clk;
clk = kzalloc(sizeof(*clk), GFP_KERNEL);
- if (!clk)
- return NULL;
+ if (!clk) {
+ pr_err(%s: could not allocate clk\n, __func__);
+ goto fail_out;
+ }
clk-name = name;
clk-ops = ops;
clk-hw = hw;
clk-flags = flags;
- clk-parent_names = parent_names;
clk-num_parents = num_parents;
hw-clk = clk;
- __clk_init(dev, clk);
+ /* allocate local copy in case parent_names is __initdata */
+ clk-parent_names = kzalloc((sizeof(char*) * num_parents),
+ GFP_KERNEL);
+
+ if (!clk-parent_names) {
+ pr_err(%s: could not allocate clk-parent_names\n, __func__);
+ goto fail_parent_names;
+ }
+
+ /* copy each string name in case parent_names is __initdata */
... to here.
The rationale is that when this code is changed later someone might use
ret above and doesn't
Re: [PATCH 13/13] clk: basic: improve parent_names return errors
On Wed, Apr 11, 2012 at 06:02:51PM -0700, Mike Turquette wrote:
This patch is the basic clk version of 'clk: core: copy parent_names
return error codes'.
The registration functions are changed to allow the core code to copy
the array of strings and allow platforms to declare those arrays as
__initdata.
This patch also converts all of the basic clk registration functions to
return error codes which better aligns them with the existing clk.h api.
+ */
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,
u8 clk_divider_flags, spinlock_t *lock)
{
struct clk_divider *div;
- struct clk *clk;
+ struct clk *clk = ERR_PTR(-ENOMEM);
+ const char *parent_names[1];
+ /* allocate the divider */
div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
-
if (!div) {
pr_err(%s: could not allocate divider clk\n, __func__);
return NULL;
You missed a conversion to ERR_PTR here.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 00/13] common clk framework misc fixes
On Wed, Apr 11, 2012 at 06:02:38PM -0700, Mike Turquette wrote: This series collects many of the fixes posted for the recently merged common clock framework as well as some general clean-up. Most of the code classifies as a clean-up moreso than a bug fix; hopefully this is not a problem since the common clk framework is new code pulled for 3.4. Patches are based on v3.4-rc2 and can be pulled from: git://git.linaro.org/people/mturquette/linux.git v3.4-rc2-clk-fixes Please let me know I missed any critical fixes that were posted to the list already. Arnd Olof, if there are no objections to these changes can this get pulled through the arm-soc tree? I had a look at this series and except for the comment Shawn already made I am fine with it. I also vote for queuing it during -rc since the framework currently has no users and letting it into -rc will help people like me putting SoC support onto it. Except the last patch: Acked-by: Sascha Hauer s.ha...@pengutronix.de Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
On Wed, Mar 21, 2012 at 01:44:01AM -0600, Paul Walmsley wrote: Hello Saravana, Certainly a Kconfig help text change seems trivial enough. But even the resistance to CONFIG_EXPERIMENTAL has been quite surprising to me, given that every single defconfig in arch/arm/defconfig sets it: $ find arch/arm/configs -type f | wc -l 122 $ fgrep -r CONFIG_EXPERIMENTAL=y arch/arm/configs | wc -l 122 $ (that includes iMX, by the way...) Certainly, neither Kconfig change is going to prevent us on OMAP from figuring out what else is needed to convert our platform to the common clock code. And given the level of enthusiasm on the lists, I don't think it's going to prevent many of the other ARM platforms from experimenting with the conversion, either. So it would be interesting to know more about why you (or anyone else) perceive that the Kconfig changes would be harmful. Mainly because COMMON_CLK is an invisible option which has to be selected by architectures. So with the Kconfig change we either have to: config ARCH_MXC depends on EXPERIMENTAL or: config ARCH_MXC select EXPERIMENTAL select COMMON_CLK Neither of both seems very appealing to me. You can add a warning to the Kconfig help text if you like, I have no problem with that. As you said it will prevent noone from using it anyway. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Mon, Mar 19, 2012 at 03:01:17PM +0800, Shawn Guo wrote: On Fri, Mar 09, 2012 at 10:25:00AM -0800, Turquette, Mike wrote: ... However if you have the ability to use the clk_foo_register functions please do use them in place of static initialization. The static init stuff is only for folks backed into a corner and forced to use it... for now. I'm looking at ways to allow for kmalloc'ing in early boot, as well as reducing the number of clocks that my platform registers during early boot drastically. While I agree using registration functions rather than static initialization will help make struct clk an opaque cookie, I also see some benefit with using static initialization over registration functions. That is we will be able to initialize parents statically rather than calling expensive __clk_lookup() to find them when using registration functions. I'm not sure if this will be a concern with the platforms that have hundreds of clocks. Keep it in mind, when we say one clock, there are generally 3 clks behind it, clk_gate, clk_divider and clk_mux. On an i.MX51 with a fully dynamically allocated clock tree it takes about 10ms to initialize the tree which I think is acceptable. The clock tree is not complete, but I would think that about 70% of the clocks are there. Normally less performant platforms will have less clocks, so I assume the times will be comparable. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On Mon, Mar 19, 2012 at 04:52:05PM +0530, Rajendra Nayak wrote:
Hi Mike,
+/*
+ * calculate the new rates returning the topmost clock that has to be
+ * changed.
+ */
+static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
+{
+struct clk *top = clk;
+unsigned long best_parent_rate = clk-parent-rate;
Shouldn't you check for a valid parent before dereferencing it? A
clk_set_rate() on a root clock might throw up some issues otherwise.
Yes, should be checked.
+unsigned long new_rate;
+
+if (!clk-ops-round_rate !(clk-flags CLK_SET_RATE_PARENT)) {
+clk-new_rate = clk-rate;
+return NULL;
So does this mean a clk_set_rate() fails for a clk which does not have
a valid .round_rate and does not have a CLK_SET_RATE_PARENT flag set?
I was thinking this could do a..
clk-new_rate = rate;
top = clk;
goto out;
..instead.
The core should make sure that either both set_rate and round_rate are
present or none of them.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
On Sat, Mar 17, 2012 at 11:02:11AM -0700, Turquette, Mike wrote: On Sat, Mar 17, 2012 at 2:05 AM, Arnd Bergmann a...@arndb.de wrote: On Friday 16 March 2012, Turquette, Mike wrote: On Fri, Mar 16, 2012 at 3:21 PM, Paul Walmsley p...@pwsan.com wrote: From: Paul Walmsley p...@pwsan.com Date: Fri, 16 Mar 2012 16:06:30 -0600 Subject: [PATCH] clk: mark the common clk code as EXPERIMENTAL for now Mark the common clk code as depending on CONFIG_EXPERIMENTAL. The API is not well-defined and both it and the underlying mechanics are likely to need significant changes to support non-trivial uses of the rate changing code, such as DVFS with external I/O devices. So any platforms that switch their implementation over to this may need to revise much of their driver code and revalidate their implementations until the behavior of the code is better-defined. A good time for removing this EXPERIMENTAL designation would be after at least two platforms that do DVFS on groups of external I/O devices have ported their clock implementations over to the common clk code. Signed-off-by: Paul Walmsley p...@pwsan.com Cc: Mike Turquette mturque...@ti.com ACK. This will set some reasonable expectations while things are in flux. Arnd are you willing to take this in? I think it's rather pointless, because the option is not going to be user selectable but will get selected by the platform unless I'm mistaken. The platform maintainers that care already know the state of the framework. Also, there are no user space interfaces that we have to warn users about not being stable, because the framework is internal to the kernel. The consensus seems to be to not set experimental. However, I wonder whether we need the patch below to prevent users from accidentally enabling COMMON_CLK on platforms that already provide their own implementation. Arnd diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 2eaf17e..a0a83de 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,7 +12,7 @@ config HAVE_MACH_CLKDEV menuconfig COMMON_CLK - bool Common Clock Framework + bool select HAVE_CLK_PREPARE ---help--- The common clock framework is a single definition of struct clk, useful across many platforms, as well as an Much like experimental I'm not sure how needed this change is. The help section does say to leave it disabled by default, if unsure. If you merge it I won't object but this might be fixing an imaginary problem. Architectures without common clock support won't build with this option enabled (multiple definition of clk_enable etc), so I think this should not be user visible. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 0/3] common clk framework
On Thu, Mar 15, 2012 at 11:11:17PM -0700, Mike Turquette wrote: The common clock framework defines a common struct clk as well as an implementation of the clk api that unifies clock operations on various platforms and devices. The net result is consolidation of many different struct clk definitions and platform-specific clock framework implementations. This series is the feature freeze for the common clock framework. Unless any major bugs are reported this should be the final version of this patchset. Now is the time to add any acked-by's, reviewed-by's or tested-by's. I've carried over the *-by's from v6; I hope everyone is OK with that. Nice work, thanks again Mike. I gave it a test on various i.MX SoCs and I can give you my: Tested-by: Sascha Hauer s.ha...@pengutronix.de Acked-by: Sascha Hauer s.ha...@pengutronix.de Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
Hi Paul, On Fri, Mar 16, 2012 at 04:21:17PM -0600, Paul Walmsley wrote: Hi On Fri, 16 Mar 2012, Arnd Bergmann wrote: If the common clock code is to go upstream now, it should be marked as experimental. No, please don't do this. This effectively marks the architectures using the generic clock framework experimental. We can mark drivers as 'you have been warned', but marking an architecture as experimental is the wrong sign for both its users and people willing to adopt the framework. Also we get this: warning: (ARCH_MX1 MACH_MX21 ARCH_MX25 MACH_MX27) selects COMMON_CLK which has unmet direct dependencies (EXPERIMENTAL) (and no, I don't want to support to clock frameworks in parallel) This is because we know the API is not well-defined, and that both the API and the underlying mechanics will almost certainly need to change for non-trivial uses of the rate changing code (e.g., DVFS with external I/O devices). Please leave DVFS out of the game. DVFS will use the clock framework for the F part and the regulator framework for the V part, but the clock framework should not get extended with DVFS features. The notifiers we currently have in the clock framework should give enough information for DVFS implementations. Even if they don't and we have to change something here this will have no influence on the architectures implementing their clock tree with the common clock framework. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Wed, Mar 14, 2012 at 05:51:48PM -0700, Turquette, Mike wrote:
On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Mon, Mar 12, 2012 at 08:16:36PM -0700, Turquette, Mike wrote:
On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer s.ha...@pengutronix.de
wrote:
I tried another
approach on the weekend which basically does not try to do all in a
single recursion but instead sets the rate in multiple steps:
1) call a function which calculates all new rates of affected clocks
in a rate change and safes the value in a clk-new_rate field. This
function returns the topmost clock which has to be changed.
2) starting from the topmost clock notify all clients. This walks the
whole subtree even if a notfifier refuses the change. If necessary
we can walk the whole subtree again to abort the change.
3) actually change rates starting from the topmost clocks and notify
all clients on the way. I changed the set_rate callback to void.
Instead of failing (what is failing in case of set_rate? The clock
will still have some rate) I check for the result with
clk_ops-recalc_rate.
The way described above works for me now, see this branch:
git://git.pengutronix.de/git/imx/linux-2.6.git v3.3-rc6-clkv6-fixup
You may not necessarily like it as it changes quite a lot in the rate
changing code.
I tried that code and I really like it! It is much more readable and
feels less fragile than the previous recursive __clk_set_rate. I
did quite a bit of testing with this code today. One of the tests
looks like this:
pll (adjustable to anything)
|
clk_divider (5 bits wide)
|
dummy(no clk_ops)
The new code did a fine job arbitrating rates for the PLL and the
intermediate divider even if I put weird constraints on the PLL. For
instance if I artificially limited it to a minimum of 600MHz and then
ran clk_set_rate(dummy, 300MHz) it would lock at 600MHz and set
clk_divider to divide-by-2. Setting to 600MHz or more set the divider
back to 1 and relocked the PLL appropriately. Pretty cool.
I also tested the notifiers with this code and they seem to function
properly. I'll take this code in for v7. Thanks a lot for this
helpful contribution.
I did find that MULT_ROUND_UP caused trouble for my PLL's round_rate
implementation. Maybe my PLL code is fragile but a quick fix was to
make sure that we send the exact value we want to the round_rate code.
I also feel this is more correct. Let me know what you think:
8---
commit 189fecedb175d0366759246c4192f45b0bc39a50
Author: Mike Turquette mturque...@linaro.org
Date: Wed Mar 14 17:29:51 2012 -0700
clk-divider.c: round the actual rate we care about
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 86ca9cd..06ef4a0 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -47,12 +47,6 @@ static unsigned long clk_divider_recalc_rate(struct
clk_hw *hw,
}
EXPORT_SYMBOL_GPL(clk_divider_recalc_rate);
-/*
- * The reverse of DIV_ROUND_UP: The maximum number which
- * divided by m is r
- */
-#define MULT_ROUND_UP(r, m) ((r) * (m) + (m) - 1)
-
static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
unsigned long *best_parent_rate)
{
@@ -84,9 +78,9 @@ static int clk_divider_bestdiv(struct clk_hw *hw,
unsigned long rate,
for (i = 1; i = maxdiv; i++) {
parent_rate = __clk_round_rate(__clk_get_parent(hw-clk),
- MULT_ROUND_UP(rate, i));
+ (rate * i));
I think MULT_ROUND_UP is the right thing to use here (not sure if this
is a good name though)
Consider we want to have an output rate of 33Hz. Now acceptable input
rates for a divider value of 3 would be 99, 100 and 101Hz, so we have
to call round_rate for the parent with 101Hz which includes 100 and
99Hz.
If you have problems with your PLL than most likely because it does
something different on clk_round_rate than it does in clk_set_rate,
for example clk_round_rate(1) returns 1, but clk_set_rate then
sets the rate due to some rounding error. Being consistent between
round_rate and set_rate is very important for this mechanism to work
properly. It did cost me some nerves to get it right for the divider
(and even more nerves to figure out why it is correct the way it works)
now = parent_rate / i;
- if (now = rate now = best) {
+ if (now = rate now best) {
This change is an optimization, but should be unrelated to your PLL
problem, right?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Tue, Mar 13, 2012 at 04:43:57PM -0700, Turquette, Mike wrote: On Tue, Mar 13, 2012 at 4:24 AM, Sascha Hauer s.ha...@pengutronix.de wrote: On Sat, Mar 03, 2012 at 12:29:00AM -0800, Mike Turquette wrote: The common clock framework defines a common struct clk useful across most platforms as well as an implementation of the clk api that drivers can use safely for managing clocks. The net result is consolidation of many different struct clk definitions and platform-specific clock framework implementations. This patch introduces the common struct clk, struct clk_ops and an implementation of the well-known clock api in include/clk/clk.h. Platforms may define their own hardware-specific clock structure and their own clock operation callbacks, so long as it wraps an instance of struct clk_hw. See Documentation/clk.txt for more details. This patch is based on the work of Jeremy Kerr, which in turn was based on the work of Ben Herrenschmidt. Signed-off-by: Mike Turquette mturque...@linaro.org Signed-off-by: Mike Turquette mturque...@ti.com Cc: Jeremy Kerr jeremy.k...@canonical.com Cc: Thomas Gleixner t...@linutronix.de Cc: Arnd Bergman arnd.bergm...@linaro.org Cc: Paul Walmsley p...@pwsan.com Cc: Shawn Guo shawn@freescale.com Cc: Richard Zhao richard.z...@linaro.org Cc: Saravana Kannan skan...@codeaurora.org Cc: Magnus Damm magnus.d...@gmail.com Cc: Rob Herring rob.herr...@calxeda.com Cc: Mark Brown broo...@opensource.wolfsonmicro.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Stephen Boyd sb...@codeaurora.org Cc: Amit Kucheria amit.kuche...@linaro.org Cc: Deepak Saxena dsax...@linaro.org Cc: Grant Likely grant.lik...@secretlab.ca Cc: Andrew Lunn and...@lunn.ch --- drivers/clk/Kconfig | 28 + drivers/clk/Makefile | 1 + drivers/clk/clk.c | 1323 ++ include/linux/clk-private.h | 68 +++ include/linux/clk-provider.h | 169 ++ include/linux/clk.h | 68 ++- 6 files changed, 1652 insertions(+), 5 deletions(-) create mode 100644 drivers/clk/clk.c create mode 100644 include/linux/clk-private.h create mode 100644 include/linux/clk-provider.h diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 3912576..18eb8c2 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -11,3 +11,31 @@ config HAVE_MACH_CLKDEV config HAVE_CLK_PREPARE bool + +menuconfig COMMON_CLK + bool Common Clock Framework + select HAVE_CLK_PREPARE + ---help--- + The common clock framework is a single definition of struct + clk, useful across many platforms, as well as an + implementation of the clock API in include/linux/clk.h. + Architectures utilizing the common struct clk should select + this automatically, but it may be necessary to manually select + this option for loadable modules requiring the common clock + framework. + + If in doubt, say N. + +if COMMON_CLK + +config COMMON_CLK_DEBUG + bool DebugFS representation of clock tree + depends on COMMON_CLK + ---help--- + Creates a directory hierchy in debugfs for visualizing the clk + tree structure. Each directory contains read-only members + that export information specific to that clk node: clk_rate, + clk_flags, clk_prepare_count, clk_enable_count + clk_notifier_count. + +endif diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 07613fa..ff362c4 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o +obj-$(CONFIG_COMMON_CLK) += clk.o diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c new file mode 100644 index 000..b979d74 --- /dev/null +++ b/drivers/clk/clk.c @@ -0,0 +1,1323 @@ +/* + * Copyright (C) 2010-2011 Canonical Ltd jeremy.k...@canonical.com + * Copyright (C) 2011-2012 Linaro Ltd mturque...@linaro.org + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * Standard functionality for the common clock API. See Documentation/clk.txt + */ + +#include linux/clk-private.h +#include linux/module.h +#include linux/mutex.h +#include linux/spinlock.h +#include linux/err.h +#include linux/list.h +#include linux/slab.h + +static DEFINE_SPINLOCK(enable_lock); +static DEFINE_MUTEX(prepare_lock); + +static HLIST_HEAD(clk_root_list); +static HLIST_HEAD(clk_orphan_list); +static LIST_HEAD(clk_notifier_list); + +/*** debugfs support ***/ + +#ifdef CONFIG_COMMON_CLK_DEBUG +#include linux/debugfs.h + +static struct dentry *rootdir; +static struct dentry *orphandir; +static
Re: [PATCH v6 3/3] clk: basic clock hardware types
On Fri, Mar 09, 2012 at 11:54:24PM -0800, Mike Turquette wrote:
Many platforms support simple gateable clocks, fixed-rate clocks,
adjustable divider clocks and multi-parent multiplexer clocks.
This patch introduces basic clock types for the above-mentioned hardware
which share some common characteristics.
Based on original work by Jeremy Kerr and contribution by Jamie Iles.
Dividers and multiplexor clocks originally contributed by Richard Zhao
Sascha Hauer.
Signed-off-by: Mike Turquette mturque...@linaro.org
Signed-off-by: Mike Turquette mturque...@ti.com
Cc: Russell King li...@arm.linux.org.uk
Cc: Jeremy Kerr jeremy.k...@canonical.com
Cc: Thomas Gleixner t...@linutronix.de
Cc: Arnd Bergman arnd.bergm...@linaro.org
Cc: Paul Walmsley p...@pwsan.com
Cc: Shawn Guo shawn@freescale.com
Cc: Sascha Hauer s.ha...@pengutronix.de
Cc: Jamie Iles ja...@jamieiles.com
Cc: Richard Zhao richard.z...@linaro.org
Cc: Saravana Kannan skan...@codeaurora.org
Cc: Magnus Damm magnus.d...@gmail.com
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Linus Walleij linus.wall...@stericsson.com
Cc: Stephen Boyd sb...@codeaurora.org
Cc: Amit Kucheria amit.kuche...@linaro.org
Cc: Deepak Saxena dsax...@linaro.org
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Andrew Lunn and...@lunn.ch
---
Changes since v5:
* standardized the hw-specific locking in the basic clock types
* export the individual ops for each basic clock type
* improve registration for single-parent basic clocks (thanks Sascha)
* fixed bugs in gate clock's static initializers (thanks Andrew)
drivers/clk/Makefile |3 +-
drivers/clk/clk-divider.c| 204
++
drivers/clk/clk-fixed-rate.c | 82 +
drivers/clk/clk-gate.c | 157
drivers/clk/clk-mux.c| 123 +
include/linux/clk-private.h | 124 +
include/linux/clk-provider.h | 127 ++
7 files changed, 819 insertions(+), 1 deletions(-)
create mode 100644 drivers/clk/clk-divider.c
create mode 100644 drivers/clk/clk-fixed-rate.c
create mode 100644 drivers/clk/clk-gate.c
create mode 100644 drivers/clk/clk-mux.c
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index ff362c4..1f736bc 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -1,3 +1,4 @@
obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o
-obj-$(CONFIG_COMMON_CLK) += clk.o
+obj-$(CONFIG_COMMON_CLK) += clk.o clk-fixed-rate.o clk-gate.o \
+clk-mux.o clk-divider.o
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
new file mode 100644
index 000..c0c4e0b
--- /dev/null
+++ b/drivers/clk/clk-divider.c
@@ -0,0 +1,204 @@
+/*
+ * Copyright (C) 2011 Sascha Hauer, Pengutronix s.ha...@pengutronix.de
+ * Copyright (C) 2011 Richard Zhao, Linaro richard.z...@linaro.org
+ * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd mturque...@linaro.org
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Adjustable divider clock implementation
+ */
+
+#include linux/clk-provider.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/io.h
+#include linux/err.h
+#include linux/string.h
+
+/*
+ * DOC: basic adjustable divider clock that cannot gate
+ *
+ * Traits of this clock:
+ * prepare - clk_prepare only ensures that parents are prepared
+ * enable - clk_enable only ensures that parents are enabled
+ * rate - rate is adjustable. clk-rate = parent-rate / divisor
+ * parent - fixed parent. No clk_set_parent support
+ */
+
+#define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
+
+static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct clk_divider *divider = to_clk_divider(hw);
+ unsigned int div;
+ unsigned long flags = 0;
+
+ if (divider-lock)
+ spin_lock_irqsave(divider-lock, flags);
+
+ div = readl(divider-reg) divider-shift;
+ div = (1 divider-width) - 1;
+
+ if (divider-lock)
+ spin_unlock_irqrestore(divider-lock, flags);
+
+ if (!(divider-flags CLK_DIVIDER_ONE_BASED))
+ div++;
+
+ return parent_rate / div;
+}
+EXPORT_SYMBOL_GPL(clk_divider_recalc_rate);
+
+static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
+ unsigned long *best_parent_rate)
+{
+ struct clk_divider *divider = to_clk_divider(hw);
+ int i, bestdiv = 0;
+ unsigned long parent_rate, best = 0, now, maxdiv;
+
+ maxdiv = (1 divider-width);
We need a check for rate == 0 here:
if (rate == 0)
rate = 1
Re: [PATCH v6 3/3] clk: basic clock hardware types
On Mon, Mar 12, 2012 at 10:50:09PM -0500, Matt Sealey wrote: Hi Mike, Can I suggest/we discuss that we support fractional (i.e. represented by fixed point value with integer and fractional part) dividers in the common divider clock case, simplistically just adding a divider fractional width and shifting all the calculations by it (and fixing the maxdiv calculation as in a fractional case width of the divider area in the register is not an indicator of the actual maximum divider)? (I guess for the standard integer divider case, it would be continually shifting by 0 which might make the code a bit bigger, I don't think that would truly be a concern though?) I have patches for this, see git://git.pengutronix.de/git/imx/linux-2.6.git imx/work/imx-clkv6 Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Sat, Mar 03, 2012 at 12:29:00AM -0800, Mike Turquette wrote:
The common clock framework defines a common struct clk useful across
most platforms as well as an implementation of the clk api that drivers
can use safely for managing clocks.
The net result is consolidation of many different struct clk definitions
and platform-specific clock framework implementations.
This patch introduces the common struct clk, struct clk_ops and an
implementation of the well-known clock api in include/clk/clk.h.
Platforms may define their own hardware-specific clock structure and
their own clock operation callbacks, so long as it wraps an instance of
struct clk_hw.
See Documentation/clk.txt for more details.
This patch is based on the work of Jeremy Kerr, which in turn was based
on the work of Ben Herrenschmidt.
Signed-off-by: Mike Turquette mturque...@linaro.org
Signed-off-by: Mike Turquette mturque...@ti.com
Cc: Jeremy Kerr jeremy.k...@canonical.com
Cc: Thomas Gleixner t...@linutronix.de
Cc: Arnd Bergman arnd.bergm...@linaro.org
Cc: Paul Walmsley p...@pwsan.com
Cc: Shawn Guo shawn@freescale.com
Cc: Richard Zhao richard.z...@linaro.org
Cc: Saravana Kannan skan...@codeaurora.org
Cc: Magnus Damm magnus.d...@gmail.com
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Linus Walleij linus.wall...@stericsson.com
Cc: Stephen Boyd sb...@codeaurora.org
Cc: Amit Kucheria amit.kuche...@linaro.org
Cc: Deepak Saxena dsax...@linaro.org
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Andrew Lunn and...@lunn.ch
---
drivers/clk/Kconfig | 28 +
drivers/clk/Makefile |1 +
drivers/clk/clk.c| 1323
++
include/linux/clk-private.h | 68 +++
include/linux/clk-provider.h | 169 ++
include/linux/clk.h | 68 ++-
6 files changed, 1652 insertions(+), 5 deletions(-)
create mode 100644 drivers/clk/clk.c
create mode 100644 include/linux/clk-private.h
create mode 100644 include/linux/clk-provider.h
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 3912576..18eb8c2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -11,3 +11,31 @@ config HAVE_MACH_CLKDEV
config HAVE_CLK_PREPARE
bool
+
+menuconfig COMMON_CLK
+ bool Common Clock Framework
+ select HAVE_CLK_PREPARE
+ ---help---
+ The common clock framework is a single definition of struct
+ clk, useful across many platforms, as well as an
+ implementation of the clock API in include/linux/clk.h.
+ Architectures utilizing the common struct clk should select
+ this automatically, but it may be necessary to manually select
+ this option for loadable modules requiring the common clock
+ framework.
+
+ If in doubt, say N.
+
+if COMMON_CLK
+
+config COMMON_CLK_DEBUG
+ bool DebugFS representation of clock tree
+ depends on COMMON_CLK
+ ---help---
+ Creates a directory hierchy in debugfs for visualizing the clk
+ tree structure. Each directory contains read-only members
+ that export information specific to that clk node: clk_rate,
+ clk_flags, clk_prepare_count, clk_enable_count
+ clk_notifier_count.
+
+endif
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 07613fa..ff362c4 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o
+obj-$(CONFIG_COMMON_CLK) += clk.o
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
new file mode 100644
index 000..b979d74
--- /dev/null
+++ b/drivers/clk/clk.c
@@ -0,0 +1,1323 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd jeremy.k...@canonical.com
+ * Copyright (C) 2011-2012 Linaro Ltd mturque...@linaro.org
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Standard functionality for the common clock API. See
Documentation/clk.txt
+ */
+
+#include linux/clk-private.h
+#include linux/module.h
+#include linux/mutex.h
+#include linux/spinlock.h
+#include linux/err.h
+#include linux/list.h
+#include linux/slab.h
+
+static DEFINE_SPINLOCK(enable_lock);
+static DEFINE_MUTEX(prepare_lock);
+
+static HLIST_HEAD(clk_root_list);
+static HLIST_HEAD(clk_orphan_list);
+static LIST_HEAD(clk_notifier_list);
+
+/***debugfs support***/
+
+#ifdef CONFIG_COMMON_CLK_DEBUG
+#include linux/debugfs.h
+
+static struct dentry *rootdir;
+static struct dentry *orphandir;
+static int inited = 0;
+
+/* caller must hold prepare_lock */
+static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry)
+{
+ struct dentry *d;
+ int ret = -ENOMEM;
+
+ if (!clk || !pdentry) {
+ ret = -EINVAL;
+ goto
Re: [PATCH v6 2/3] clk: introduce the common clock framework
Hi Mike,
On Mon, Mar 12, 2012 at 08:16:36PM -0700, Turquette, Mike wrote:
On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote:
On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer s.ha...@pengutronix.de
wrote:
Hi Mike,
I was about to give my tested-by when I decided to test the set_rate
function. Unfortunately this is broken for several reasons. I'll try
to come up with a fixup series later the day.
I haven't tested clk_set_rate since V4, but I also haven't changed the
code appreciably. I'll retest on my end also.
On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
+ /* find the new rate and see if parent rate should change too */
+ WARN_ON(!clk-ops-round_rate);
+
+ new_rate = clk-ops-round_rate(clk-hw, rate, parent_new_rate);
You don't need a WARN_ON when you derefence clk-ops-round_rate anyway.
Agreed that the WARN_ON should not be there.
The v6 Documentation/clk.txt states that .round_rate is mandatory for
clocks that can adjust their rate, but I need to clarify this a bit
more. Ideally we want to be able to call clk_set_rate on any clock
and get a changed rate (if possible) by either adjusting that clocks
rate direction (e.g. a PLL or an adjustable divider) or by propagating
__clk_set_rate up the parents (assuming of course that
CLK_SET_RATE_PARENT flag is set appropriately).
Also, even when the current clock does not have a set_rate function it
can still change its rate when the CLK_SET_RATE_PARENT is set.
Correct. I'll clean this up and make the documentation a bit more
verbose on when .set_rate/.round_rate/.recalc_rate are mandatory.
+
+ /* NOTE: pre-rate change notifications will stack */
+ if (clk-notifier_count)
+ ret = __clk_notify(clk, PRE_RATE_CHANGE, clk-rate,
new_rate);
+
+ if (ret == NOTIFY_BAD)
+ return clk;
+
+ /* speculate rate changes down the tree */
+ hlist_for_each_entry(child, tmp, clk-children, child_node) {
+ ret = __clk_speculate_rates(child, new_rate);
+ if (ret == NOTIFY_BAD)
+ return clk;
+ }
+
+ /* change the rate of this clk */
+ if (clk-ops-set_rate)
+ ret = clk-ops-set_rate(clk-hw, new_rate);
I don't know the reason why you change the child clock before the parent
clock, but it cannot work since this clock will change its rate based on
the old parent rate and not the new one.
This depends on the .round_rate implementation, which I admit to
having lost some sleep over. A clever .round_rate will request the
intermediate rate for a clock when propagating a request to change
the parent rate later on. Take for instance the following:
pll @ 200MHz (locked)
|
parent @ 100MHz (can divide by 1 or 2; currently divider is 2)
|
child @ 25MHz (can divide by 2 or 4; currently divider is 4)
If we want child to run at 100MHz then the desirable configuration
would be to have parent divide-by-1 and child divide-by-2. When we
call,
clk_set_rate(child, 100MHz);
Its .round_rate should return 50MHz, and parent_new_rate should be
200MHz. So 50MHz is an intermediate rate, but it gets us the
divider we want. And in fact 50MHz reflects reality because that will
be the rate of child until the parent propagation completes and we can
adjust parent's dividers. (this is one reason why I prefer for
pre-rate change notifiers to stack on top of each other).
So now that parent_new_rate is 0, __clk_set_rate will propagate the
request up and parent's .round_rate will simply return 200MHz and
leave it's own parent_new_rate at 0. This will change from
divide-by-2 to divide-by-1 and from this highest point in the tree we
will propagate post-rate change notifiers downstream, as part of the
recalc_rate tree walk.
I have tested this with OMAP4's CPUfreq driver and I think, while
complicated, it is a sound way to approach the problem. Maybe the API
can be cleaned up, if you have any suggestions.
I cannot see all implications this way will have. All this rate
propagation is more complex than I thought it would be.
Hi Sascha,
Yes it is very complicated. The solution I have now (recursive
__clk_set_rate, clever .round_rate which requests parent rate) was not
something I arrived at immediately.
I decided to validate the v6 patches more thoroughly today, based on
your claim that clk_set_rate is broken and here is what I found:
1) clk_set_rate works. I pulled in the latest OMAP4 CPUfreq code into
my common clk branch and it Just Worked. This is a dumb
implementation involving no upwards parent propagation, and the clock
changing is of type struct clk_hw_omap (relocking a PLL)
2) while I was at it I verified the rate change
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote:
On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
Hi Mike,
I was about to give my tested-by when I decided to test the set_rate
function. Unfortunately this is broken for several reasons. I'll try
to come up with a fixup series later the day.
I haven't tested clk_set_rate since V4, but I also haven't changed the
code appreciably. I'll retest on my end also.
On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
+ /* find the new rate and see if parent rate should change too */
+ WARN_ON(!clk-ops-round_rate);
+
+ new_rate = clk-ops-round_rate(clk-hw, rate, parent_new_rate);
You don't need a WARN_ON when you derefence clk-ops-round_rate anyway.
Agreed that the WARN_ON should not be there.
The v6 Documentation/clk.txt states that .round_rate is mandatory for
clocks that can adjust their rate, but I need to clarify this a bit
more. Ideally we want to be able to call clk_set_rate on any clock
and get a changed rate (if possible) by either adjusting that clocks
rate direction (e.g. a PLL or an adjustable divider) or by propagating
__clk_set_rate up the parents (assuming of course that
CLK_SET_RATE_PARENT flag is set appropriately).
Also, even when the current clock does not have a set_rate function it
can still change its rate when the CLK_SET_RATE_PARENT is set.
Correct. I'll clean this up and make the documentation a bit more
verbose on when .set_rate/.round_rate/.recalc_rate are mandatory.
+
+ /* NOTE: pre-rate change notifications will stack */
+ if (clk-notifier_count)
+ ret = __clk_notify(clk, PRE_RATE_CHANGE, clk-rate,
new_rate);
+
+ if (ret == NOTIFY_BAD)
+ return clk;
+
+ /* speculate rate changes down the tree */
+ hlist_for_each_entry(child, tmp, clk-children, child_node) {
+ ret = __clk_speculate_rates(child, new_rate);
+ if (ret == NOTIFY_BAD)
+ return clk;
+ }
+
+ /* change the rate of this clk */
+ if (clk-ops-set_rate)
+ ret = clk-ops-set_rate(clk-hw, new_rate);
I don't know the reason why you change the child clock before the parent
clock, but it cannot work since this clock will change its rate based on
the old parent rate and not the new one.
This depends on the .round_rate implementation, which I admit to
having lost some sleep over. A clever .round_rate will request the
intermediate rate for a clock when propagating a request to change
the parent rate later on. Take for instance the following:
pll @ 200MHz (locked)
|
parent @ 100MHz (can divide by 1 or 2; currently divider is 2)
|
child @ 25MHz (can divide by 2 or 4; currently divider is 4)
If we want child to run at 100MHz then the desirable configuration
would be to have parent divide-by-1 and child divide-by-2. When we
call,
clk_set_rate(child, 100MHz);
Its .round_rate should return 50MHz, and parent_new_rate should be
200MHz. So 50MHz is an intermediate rate, but it gets us the
divider we want. And in fact 50MHz reflects reality because that will
be the rate of child until the parent propagation completes and we can
adjust parent's dividers. (this is one reason why I prefer for
pre-rate change notifiers to stack on top of each other).
So now that parent_new_rate is 0, __clk_set_rate will propagate the
request up and parent's .round_rate will simply return 200MHz and
leave it's own parent_new_rate at 0. This will change from
divide-by-2 to divide-by-1 and from this highest point in the tree we
will propagate post-rate change notifiers downstream, as part of the
recalc_rate tree walk.
I have tested this with OMAP4's CPUfreq driver and I think, while
complicated, it is a sound way to approach the problem. Maybe the API
can be cleaned up, if you have any suggestions.
I cannot see all implications this way will have. All this rate
propagation is more complex than I thought it would be. I tried another
approach on the weekend which basically does not try to do all in a
single recursion but instead sets the rate in multiple steps:
1) call a function which calculates all new rates of affected clocks
in a rate change and safes the value in a clk-new_rate field. This
function returns the topmost clock which has to be changed.
2) starting from the topmost clock notify all clients. This walks the
whole subtree even if a notfifier refuses the change. If necessary
we can walk the whole subtree again to abort the change.
3) actually change rates starting from the topmost clocks and notify
all clients on the way. I changed the set_rate callback to void.
Instead of failing (what is failing in case of set_rate? The clock
will still have some rate) I check for the result with
clk_ops-recalc_rate.
In the end what's more important than
Re: [PATCH v6 2/3] clk: introduce the common clock framework
Hi Mike,
I was about to give my tested-by when I decided to test the set_rate
function. Unfortunately this is broken for several reasons. I'll try
to come up with a fixup series later the day.
On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
+
+/**
+ * DOC: Using the CLK_SET_RATE_PARENT flag
+ *
+ * __clk_set_rate changes the child's rate before the parent's to more
+ * easily handle failure conditions.
+ *
+ * This means clk might run out of spec for a short time if its rate is
+ * increased before the parent's rate is updated.
+ *
+ * To prevent this consider setting the CLK_SET_RATE_GATE flag on any
+ * clk where you also set the CLK_SET_RATE_PARENT flag
+ *
+ * PRE_RATE_CHANGE notifications are supposed to stack as a rate change
+ * request propagates up the clk tree. This reflects the different
+ * rates that a downstream clk might experience if left enabled while
+ * upstream parents change their rates.
+ */
+static struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ struct clk *fail_clk = NULL;
+ int ret = NOTIFY_DONE;
+ unsigned long old_rate = clk-rate;
+ unsigned long new_rate;
+ unsigned long parent_old_rate;
+ unsigned long parent_new_rate = 0;
+ struct clk *child;
+ struct hlist_node *tmp;
+
+ /* bail early if we can't change rate while clk is enabled */
+ if ((clk-flags CLK_SET_RATE_GATE) clk-enable_count)
+ return clk;
+
+ /* find the new rate and see if parent rate should change too */
+ WARN_ON(!clk-ops-round_rate);
+
+ new_rate = clk-ops-round_rate(clk-hw, rate, parent_new_rate);
You don't need a WARN_ON when you derefence clk-ops-round_rate anyway.
Also, even when the current clock does not have a set_rate function it
can still change its rate when the CLK_SET_RATE_PARENT is set.
+
+ /* NOTE: pre-rate change notifications will stack */
+ if (clk-notifier_count)
+ ret = __clk_notify(clk, PRE_RATE_CHANGE, clk-rate, new_rate);
+
+ if (ret == NOTIFY_BAD)
+ return clk;
+
+ /* speculate rate changes down the tree */
+ hlist_for_each_entry(child, tmp, clk-children, child_node) {
+ ret = __clk_speculate_rates(child, new_rate);
+ if (ret == NOTIFY_BAD)
+ return clk;
+ }
+
+ /* change the rate of this clk */
+ if (clk-ops-set_rate)
+ ret = clk-ops-set_rate(clk-hw, new_rate);
I don't know the reason why you change the child clock before the parent
clock, but it cannot work since this clock will change its rate based on
the old parent rate and not the new one.
There are more things, as said I'll try to come up with a fixup series.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 0/4] common clk framework
Hi Richard,
On Fri, Mar 09, 2012 at 10:34:19AM +0800, Richard Zhao wrote:
Hello Mike,
The main interface for clk implementer is to register clocks dynamically.
I think it highly depends on clk DT bindings. From the patch Grant sent
out, it looks like he doesn't like one node per clk. So how do we
register clocks dynamically? You have any sample code?
Find my current work based on this series here:
git://git.pengutronix.de/git/imx/linux-2.6.git work/imx-clkv5
This implements the generic clock support for the i.MX v4/v5 based
SoCs. i.MX1 and i.MX27 are runtime tested, i.MX21/25 are compile
tested only.
A typical clock file will then look like this, here the i.MX27
implementation:
8---
ARM i.MX27: implement clocks using common clock framework
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
---
arch/arm/mach-imx/Makefile|2 +-
arch/arm/mach-imx/clk-imx27.c | 227 +
2 files changed, 228 insertions(+), 1 deletions(-)
create mode 100644 arch/arm/mach-imx/clk-imx27.c
diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
index d96e2ce..b39f2d6 100644
--- a/arch/arm/mach-imx/Makefile
+++ b/arch/arm/mach-imx/Makefile
@@ -6,7 +6,7 @@ obj-$(CONFIG_SOC_IMX21) += clk-imx21.o mm-imx21.o
obj-$(CONFIG_SOC_IMX25) += clk-imx25.o mm-imx25.o ehci-imx25.o cpu-imx25.o
obj-$(CONFIG_SOC_IMX27) += cpu-imx27.o pm-imx27.o
-obj-$(CONFIG_SOC_IMX27) += clock-imx27.o mm-imx27.o ehci-imx27.o
+obj-$(CONFIG_SOC_IMX27) += clk-imx27.o mm-imx27.o ehci-imx27.o
obj-$(CONFIG_SOC_IMX31) += mm-imx3.o cpu-imx31.o clock-imx31.o iomux-imx31.o
ehci-imx31.o
obj-$(CONFIG_SOC_IMX35) += mm-imx3.o cpu-imx35.o clock-imx35.o ehci-imx35.o
diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c
new file mode 100644
index 000..f54e4ee
--- /dev/null
+++ b/arch/arm/mach-imx/clk-imx27.c
@@ -0,0 +1,227 @@
+#include linux/clk.h
+#include linux/io.h
+#include linux/module.h
+#include linux/clkdev.h
+#include linux/clk-provider.h
+
+#include asm/div64.h
+
+#include mach/common.h
+#include mach/hardware.h
+#include clk.h
+
+#define IO_ADDR_CCM(off) (MX27_IO_ADDRESS(MX27_CCM_BASE_ADDR + (off)))
+
+/* Register offsets */
+#define CCM_CSCR IO_ADDR_CCM(0x0)
+#define CCM_MPCTL0 IO_ADDR_CCM(0x4)
+#define CCM_MPCTL1 IO_ADDR_CCM(0x8)
+#define CCM_SPCTL0 IO_ADDR_CCM(0xc)
+#define CCM_SPCTL1 IO_ADDR_CCM(0x10)
+#define CCM_OSC26MCTL IO_ADDR_CCM(0x14)
+#define CCM_PCDR0 IO_ADDR_CCM(0x18)
+#define CCM_PCDR1 IO_ADDR_CCM(0x1c)
+#define CCM_PCCR0 IO_ADDR_CCM(0x20)
+#define CCM_PCCR1 IO_ADDR_CCM(0x24)
+#define CCM_CCSR IO_ADDR_CCM(0x28)
+#define CCM_PMCTL IO_ADDR_CCM(0x2c)
+#define CCM_PMCOUNTIO_ADDR_CCM(0x30)
+#define CCM_WKGDCTLIO_ADDR_CCM(0x34)
+
+#define CCM_CSCR_UPDATE_DIS(1 31)
+#define CCM_CSCR_SSI2 (1 23)
+#define CCM_CSCR_SSI1 (1 22)
+#define CCM_CSCR_VPU (1 21)
+#define CCM_CSCR_MSHC (1 20)
+#define CCM_CSCR_SPLLRES(1 19)
+#define CCM_CSCR_MPLLRES(1 18)
+#define CCM_CSCR_SP (1 17)
+#define CCM_CSCR_MCU(1 16)
+#define CCM_CSCR_OSC26MDIV (1 4)
+#define CCM_CSCR_OSC26M (1 3)
+#define CCM_CSCR_FPM(1 2)
+#define CCM_CSCR_SPEN (1 1)
+#define CCM_CSCR_MPEN (1 0)
+
+/* i.MX27 TO 2+ */
+#define CCM_CSCR_ARM_SRC(1 15)
+
+#define CCM_SPCTL1_LF (1 15)
+#define CCM_SPCTL1_BRMO (1 6)
+
+static char *vpu_sel_clks[] = { spll, mpll_main2, };
+static char *cpu_sel_clks[] = { mpll_main2, mpll, };
+
+struct clkl {
+ struct clk_lookup lookup;
+ const char *clkname;
+};
+
+#define clkdev(d, n, c) \
+ { \
+ .lookup.dev_id = d, \
+ .lookup.con_id = n, \
+ .clkname = c, \
+ },
+
+static struct clkl lookups[] = {
+ clkdev(imx21-uart.0, ipg, uart1_ipg_gate)
+ clkdev(imx21-uart.0, per, per1_gate)
+ clkdev(imx21-uart.1, ipg, uart2_ipg_gate)
+ clkdev(imx21-uart.1, per, per1_gate)
+ clkdev(imx21-uart.2, ipg, uart3_ipg_gate)
+ clkdev(imx21-uart.2, per, per1_gate)
+ clkdev(imx21-uart.3, ipg, uart4_ipg_gate)
+ clkdev(imx21-uart.3, per, per1_gate)
+ clkdev(imx21-uart.4, ipg, uart5_ipg_gate)
+ clkdev(imx21-uart.4, per, per1_gate)
+ clkdev(imx21-uart.5, ipg, uart6_ipg_gate)
+ clkdev(imx21-uart.5, per, per1_gate)
+ clkdev(imx-gpt.0, ipg, gpt1_ipg_gate)
+ clkdev(imx-gpt.0, per, per1_gate)
+ clkdev(imx-gpt.1, ipg, gpt2_ipg_gate)
+ clkdev(imx-gpt.1, per, per1_gate)
+ clkdev(imx-gpt.2, ipg, gpt3_ipg_gate)
+ clkdev(imx-gpt.2, per, per1_gate)
+ clkdev(imx-gpt.3, ipg, gpt4_ipg_gate)
+ clkdev(imx-gpt.3, per, per1_gate
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Thu, Mar 08, 2012 at 07:27:39AM +0100, Andrew Lunn wrote: Assuming that some day OMAP code can be refactored to allow for lazy (or at least initcall-based) registration of clocks then perhaps your suggestion can take root. Which leads me to this question: are there any other platforms out there that require the level of expose to struct clk present in this patchset? OMAP does, for now, but if that changes then I need to know if others require this as well. Hi Mike For kirkwood, i use static clk's for all but my root clock. I cannot statically know the rate of the root clock, so i have to determine it at boot time using heuristics, PCI ID, etc. I used statics thinking it would be less code. No idea if it actually is, and there is nothing stopping me moving to creating the clocks after creating the root clock. I'd say use the nonstatic ones. I think using the static initializers will cause us much pain in the future. I've been through several rebases on the i.MX clock rework and everytime I wish my sed foo would be better. Now imagine what happens when it turns out that the internal struct clk layout or the structs for the muxes/dividers/gates have to be changed. This task is next to impossible when we have thousands of clocks scattered around the tree. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 4/4] clk: basic clock hardware types
On Sat, Mar 03, 2012 at 12:29:01AM -0800, Mike Turquette wrote:
+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,
+ u8 clk_divider_flags, spinlock_t *lock)
+{
+ struct clk_divider *div;
+ char **parent_names = NULL;
+ u8 len;
+
+ div = kmalloc(sizeof(struct clk_divider), GFP_KERNEL);
+
+ if (!div) {
+ pr_err(%s: could not allocate divider clk\n, __func__);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ /* struct clk_divider assignments */
+ div-reg = reg;
+ div-shift = shift;
+ div-width = width;
+ div-flags = clk_divider_flags;
+ div-lock = lock;
+
+ if (parent_name) {
+ parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
+
+ if (! parent_names)
+ goto out;
+
+ len = sizeof(char) * strlen(parent_name);
+
+ parent_names[0] = kmalloc(len, GFP_KERNEL);
+
+ if (!parent_names[0])
+ goto out;
+
+ strncpy(parent_names[0], parent_name, len);
+ }
+
+out:
+ return clk_register(dev, name,
+ clk_divider_ops, div-hw,
+ parent_names,
+ (parent_name ? 1 : 0),
+ flags);
+}
clk_register_divider and also clk_register_gate have some problems.
First you allocate memory with exactly the string length without
the terminating 0. Then the functions leak memory when clk_register
fails. Could you fold in the following patch to fix this?
Sascha
8--
fix divider/gate registration
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
---
drivers/clk/clk-divider.c| 34 +++---
drivers/clk/clk-gate.c | 33 ++---
include/linux/clk-provider.h |2 ++
3 files changed, 31 insertions(+), 38 deletions(-)
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 8f02930..99b6b55 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -156,14 +156,13 @@ struct clk *clk_register_divider(struct device *dev,
const char *name,
u8 clk_divider_flags, spinlock_t *lock)
{
struct clk_divider *div;
- char **parent_names = NULL;
- u8 len;
+ struct clk *clk;
- div = kmalloc(sizeof(struct clk_divider), GFP_KERNEL);
+ div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
if (!div) {
pr_err(%s: could not allocate divider clk\n, __func__);
- return ERR_PTR(-ENOMEM);
+ return NULL;
}
/* struct clk_divider assignments */
@@ -174,25 +173,22 @@ struct clk *clk_register_divider(struct device *dev,
const char *name,
div-lock = lock;
if (parent_name) {
- parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
-
- if (! parent_names)
- goto out;
-
- len = sizeof(char) * strlen(parent_name);
-
- parent_names[0] = kmalloc(len, GFP_KERNEL);
-
- if (!parent_names[0])
+ div-parent[0] = kstrdup(parent_name, GFP_KERNEL);
+ if (!div-parent[0])
goto out;
-
- strncpy(parent_names[0], parent_name, len);
}
-out:
- return clk_register(dev, name,
+ clk = clk_register(dev, name,
clk_divider_ops, div-hw,
- parent_names,
+ div-parent,
(parent_name ? 1 : 0),
flags);
+ if (clk)
+ return clk;
+
+out:
+ kfree(div-parent[0]);
+ kfree(div);
+
+ return NULL;
}
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index e831f7b..92c0489 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -80,14 +80,13 @@ struct clk *clk_register_gate(struct device *dev, const
char *name,
u8 clk_gate_flags, spinlock_t *lock)
{
struct clk_gate *gate;
- char **parent_names = NULL;
- u8 len;
+ struct clk *clk;
- gate = kmalloc(sizeof(struct clk_gate), GFP_KERNEL);
+ gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
if (!gate) {
pr_err(%s: could not allocate gated clk\n, __func__);
- return ERR_PTR(-ENOMEM);
+ return NULL;
}
/* struct clk_gate assignments */
@@ -97,25 +96,21 @@ struct clk *clk_register_gate(struct device *dev, const
char *name,
gate-lock = lock;
if (parent_name) {
- parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
-
- if (! parent_names)
- goto out;
-
- len = sizeof(char) * strlen
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Mon, Mar 05, 2012 at 12:03:15PM -0800, Turquette, Mike wrote:
On Sun, Mar 4, 2012 at 11:38 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sun, Mar 04, 2012 at 04:12:21PM -0800, Turquette, Mike wrote:
I believe this patch already does what you suggest, but I might be
missing your point.
In include/linux/clk-private.h you expose struct clk outside the core.
This has to be done to make static initializers possible. There is a big
warning in this file that it must not be included from files implementing
struct clk_ops. You can simply avoid this warning by declaring struct clk
with only a single member:
include/linux/clk.h:
struct clk {
struct clk_internal *internal;
};
This way everybody knows struct clk (thus can embed it in their static
initializers), but doesn't know anything about the internal members. Now
in drivers/clk/clk.c you declare struct clk_internal exactly like struct
clk was declared before:
struct clk_internal {
const char *name;
const struct clk_ops *ops;
struct clk_hw *hw;
struct clk *parent;
char **parent_names;
struct clk **parents;
u8 num_parents;
unsigned long rate;
unsigned long flags;
unsigned int enable_count;
unsigned int prepare_count;
struct hlist_head children;
struct hlist_node child_node;
unsigned int notifier_count;
#ifdef CONFIG_COMMON_CLK_DEBUG
struct dentry *dentry;
#endif
};
An instance of struct clk_internal will be allocated in
__clk_init/clk_register. Now the private data stays completely inside
the core and noone can abuse it.
Hi Sascha,
I see the disconnect here. For OMAP (and possibly other platforms) at
least some clock data is necessary during early boot, before the
regular allocation methods are available (timers for instance).
We had this problem on i.MX aswell. It turned out that the timer clock
is the only clock that is needed so early. We solved this by moving the
clock init to the system timer init function.
When you say mov[ed] the clock init to the system timer init
function do you mean that you statically allocated struct clk and
used the clk framework api, or instead you just did some direct
register writes to initialize things properly?
I meant that on i.MX we do the clock tree initialization when kmalloc is
available, see the attached patch for omap4 based on your branch which
does the same for Omap. The first clock you need is the one for the
timer, so you can initialize the clocktree at the beginning of
time_init() and don't need statically initialized clocks anymore.
Well, the file is work in progress, you probably fix this before sending
it out, but I bet people will include clk-private.h and nobody else
notices it.
clock44xx_data.c does not violate that rule. None of the logic that
implements ops for those clocks is present clock44xx_data.c.
Indeed, I missed that. It only has the ops but not the individual
functions.
All of
the code in that file is simply initialization and registration of
OMAP4 clocks. Many of the clocks are basic clock types (divider,
multiplexer and fixed-rate are used in that file) with protected code
drivers/clk/clk-*.c and the remaining clocks are of the struct
clk_hw_omap variety, which has code spread across several files:
arch/arm/mach-omap2/clock.c
arch/arm/mach-omap2/clock.h
arch/arm/mach-omap2/clkt_dpll.c
arch/arm/mach-omap2/clkt_clksel.c
arch/arm/mach-omap2/dpll3xxx.c
arch/arm/mach-omap2/dpll4xxx.c
All of the above files include linux/clk-provider.h, not
linux/clk-private.h. That code makes heavy use of the
__clk_get_whatever helpers and shows how a platform might honor the
layer of separation between struct clk and stuct clk_ops/struct
clk_foo. You are correct that the code is a work-in-progress, but
there are no layering violations that I can see.
I also think we are talking past each other to some degree. One point
I would like to make (and maybe you already know this from code
review) is that it is unnecessary to have pointers to your parent
struct clk*'s when either initializing or registering your clocks. In
fact the existing clk_register_foo functions don't even allow you to
pass in parent pointers and rely wholly on string name matching. I
just wanted to point that out in case it went unnoticed, as it is a
new way of doing things from the previous series and was born out of
Thomas' review of V4 and multi-parent handling. This also keeps
device-tree in mind where we might not know the struct clk *pointer at
compile time for connecting discrete devices.
Yes, I've seen this and I really like it. Also the change
Re: [PATCH v5 4/4] clk: basic clock hardware types
On Mon, Mar 05, 2012 at 09:48:23AM +0100, Andrew Lunn wrote: On Sun, Mar 04, 2012 at 04:30:08PM -0800, Turquette, Mike wrote: On Sun, Mar 4, 2012 at 9:42 AM, Andrew Lunn and...@lunn.ch wrote: On Sat, Mar 03, 2012 at 12:29:01AM -0800, Mike Turquette wrote: Many platforms support simple gateable clocks, fixed-rate clocks, adjustable divider clocks and multi-parent multiplexer clocks. This patch introduces basic clock types for the above-mentioned hardware which share some common characteristics. Hi Mike These basic clocks don't allow the use of prepare/unprepare, from the side of the clock provider. I think i need this. This is an interesting point and might help us nail down exactly what we expect from clk_prepare/clk_unprepare. The Orion Kirkwood SoC has clock gating for most of its on chip peripherals, which the other older Orion devices don't have. The SATA and PCIe also allows the PHY to be shut down, again which older Orion devices don't have. The current code links the clock and the PHY together, shutting both down are the same time. So i would like to perform the PHY shutdown in the unprepare() function of the clk driver. Do you feel it is The Right Thing to enable/disable the phy from clk_prepare/clk_unprepare? Humm, not sure yet. I don't know all the different possibilities, which is why i tried to describe my use case, rather than just assume i need prepare/unprepare. I also realized i did not explain my use case properly. At boot, uboot is turning on various clocks and SATA/PCIe PHYs etc, in order to get the device booted. Linux takes over, and the Orion/kirkwood board files, ask the kirkwood or generic Orion code to create platform_data structures for the different devices that the board uses. The kirkwood code keeps a bitmap of devices for which it creates platform data for which there is a gated clock. Then in a lateinit call, it turns on clocks which are needed, and also turns off clocks which are no longer needed, because the board did not ask for a driver binding for that device. If it turns off a SATA or PCIe clock, it also turns off the PHY associated with it. So we are talking about turning off hardware for which there is no driver. This seems to exclude pm_runtime_get(_sync), which is about hardware with drivers. We touched on this subject a couple of months ago, at least with respect to clocks. You said that is what the flag CLK_IGNORE_UNUSED will be used for. In a lateinit call, you plan to iterate over all clocks and turn off any which don't have CLK_IGNORE_UNUSED and have not been enabled. I assume you will call both disable() and unprepare(), and if i've can put code into the unprepare to turn the PHY off, all is good. Is allowing to pass prepare/unprepare functions to basic clocks something you want to support? If i prepare a patch would you consider it? My original instinct was no. The simple gate clock was always supposed to be simple and if you need more than it provides then it might be best for your platform to implement a specific clock type. Especially since the purpose of clk_prepare is still up in the air. I think i can wrap your simple gate clock, to make my complex gate clock. What would help is if you would EXPORT_SYMBOL_GPL clk_gate_enable() and clk_gate_disable(), since they do exactly what i want. I can then build my own clk_ops structure, with my own unprepare() function. I would probably use DEFINE_CLK_GATE as is, and then at run time, before calling __clk_init() overwrite the .ops with my own version. Maybe I don't get your point, but clk_unprepare should be used when you have to sleep to disable your clock. When clk_gate_disable() is exactly why do you want to use clk_unprepare instead of clk_disable? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Sun, Mar 04, 2012 at 04:12:21PM -0800, Turquette, Mike wrote:
I believe this patch already does what you suggest, but I might be
missing your point.
In include/linux/clk-private.h you expose struct clk outside the core.
This has to be done to make static initializers possible. There is a big
warning in this file that it must not be included from files implementing
struct clk_ops. You can simply avoid this warning by declaring struct clk
with only a single member:
include/linux/clk.h:
struct clk {
struct clk_internal *internal;
};
This way everybody knows struct clk (thus can embed it in their static
initializers), but doesn't know anything about the internal members. Now
in drivers/clk/clk.c you declare struct clk_internal exactly like struct
clk was declared before:
struct clk_internal {
const char *name;
const struct clk_ops *ops;
struct clk_hw *hw;
struct clk *parent;
char **parent_names;
struct clk **parents;
u8 num_parents;
unsigned long rate;
unsigned long flags;
unsigned int enable_count;
unsigned int prepare_count;
struct hlist_head children;
struct hlist_node child_node;
unsigned int notifier_count;
#ifdef CONFIG_COMMON_CLK_DEBUG
struct dentry *dentry;
#endif
};
An instance of struct clk_internal will be allocated in
__clk_init/clk_register. Now the private data stays completely inside
the core and noone can abuse it.
Hi Sascha,
I see the disconnect here. For OMAP (and possibly other platforms) at
least some clock data is necessary during early boot, before the
regular allocation methods are available (timers for instance).
We had this problem on i.MX aswell. It turned out that the timer clock
is the only clock that is needed so early. We solved this by moving the
clock init to the system timer init function.
Due
to this my idea of static initialization was to take care of
everything that would normally require an allocator, which includes
the internals of struct clk; thus exposing struct clk is useful here
as you can still use the clock framework during very early boot. Your
method above doesn't satisfy this requirement. I'm not sure what the
purpose would be of statically allocating your version of struct clk,
which will ultimately need to allocate memory for the clock internals
anyways. Can you elaborate the benefit of this approach over just
using the clk_foo_register functions?
As said the benefit is that you do not have to expose the internal
layout of struct clk outside the clock framework. Note that in the
file you referenced here:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/clock44xx_data.c;h=7f833a7b2dca84a52c2bd1e7c8d9cfe560771258;hb=v3.3-rc5-clkv5-omap#l205
You violate what you have in a comment above clk-private.h:
/* __clk_init is only exposed via clk-private.h and is intended for use with
* very large numbers of clocks that need to be statically initialized. It is
* a layering violation to include clk-private.h from any code which implements
* a clock's .ops; as such any statically initialized clock data MUST be in
* separate C file from the logic that implements it's operations.
*/
Well, the file is work in progress, you probably fix this before sending
it out, but I bet people will include clk-private.h and nobody else
notices it.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Sat, Mar 03, 2012 at 09:14:43AM -0800, Turquette, Mike wrote:
On Sat, Mar 3, 2012 at 5:31 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Sat, Mar 03, 2012 at 12:29:00AM -0800, Mike Turquette wrote:
The common clock framework defines a common struct clk useful across
most platforms as well as an implementation of the clk api that drivers
can use safely for managing clocks.
The net result is consolidation of many different struct clk definitions
and platform-specific clock framework implementations.
This patch introduces the common struct clk, struct clk_ops and an
implementation of the well-known clock api in include/clk/clk.h.
Platforms may define their own hardware-specific clock structure and
their own clock operation callbacks, so long as it wraps an instance of
struct clk_hw.
See Documentation/clk.txt for more details.
This patch is based on the work of Jeremy Kerr, which in turn was based
on the work of Ben Herrenschmidt.
+
+/**
+ * struct clk_hw - handle for traversing from a struct clk to its
corresponding
+ * hardware-specific structure. struct clk_hw should be declared within
struct
+ * clk_foo and then referenced by the struct clk instance that uses struct
+ * clk_foo's clk_ops
+ *
+ * clk: pointer to the struct clk instance that points back to this struct
+ * clk_hw instance
+ */
+struct clk_hw {
+ struct clk *clk;
+};
The reason for doing this is that struct clk should be an opaque cookie
for both drivers and implementers of clocks. I recently had the idea whether
the roles of these two structs could be swapped. So instead of the above we
could do:
struct clk {
struct clk_hw *hw;
}
Firstly, struct clk is an opaque cookie for both drivers and
implementers of clocks with this patchset.
Secondly, struct clk does indeed have a pointer to struct clk_hw.
Refer to include/linux/clk-private.h in this patch.
The reference is cyclical. A reference to struct clk can navigate to
struct clk_foo via container_of (usually something like #define
to_clk_foo(_hw) container_of(_hw, struct clk_foo, hw) where struct
clk's pointer to it's .hw member is passed into one of the struct
clk_ops callbacks.
Likewise if struct clk_foo needs the struct clk ptr for any reason
then it can get it from foo-hw-clk.
I believe this patch already does what you suggest, but I might be
missing your point.
In include/linux/clk-private.h you expose struct clk outside the core.
This has to be done to make static initializers possible. There is a big
warning in this file that it must not be included from files implementing
struct clk_ops. You can simply avoid this warning by declaring struct clk
with only a single member:
include/linux/clk.h:
struct clk {
struct clk_internal *internal;
};
This way everybody knows struct clk (thus can embed it in their static
initializers), but doesn't know anything about the internal members. Now
in drivers/clk/clk.c you declare struct clk_internal exactly like struct
clk was declared before:
struct clk_internal {
const char *name;
const struct clk_ops*ops;
struct clk_hw *hw;
struct clk *parent;
char**parent_names;
struct clk **parents;
u8 num_parents;
unsigned long rate;
unsigned long flags;
unsigned intenable_count;
unsigned intprepare_count;
struct hlist_head children;
struct hlist_node child_node;
unsigned intnotifier_count;
#ifdef CONFIG_COMMON_CLK_DEBUG
struct dentry *dentry;
#endif
};
An instance of struct clk_internal will be allocated in
__clk_init/clk_register. Now the private data stays completely inside
the core and noone can abuse it.
With this __clk_init could be something like:
struct clk_initializer {
const char *name;
const struct clk_ops*ops;
char**parent_names;
u8 num_parents;
unsigned long flags;
struct clk *clk;
};
void __clk_init(struct device *dev, struct clk_initializer *init);
I hope I made my intention a bit clearer.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Sat, Mar 03, 2012 at 12:29:00AM -0800, Mike Turquette wrote:
The common clock framework defines a common struct clk useful across
most platforms as well as an implementation of the clk api that drivers
can use safely for managing clocks.
The net result is consolidation of many different struct clk definitions
and platform-specific clock framework implementations.
This patch introduces the common struct clk, struct clk_ops and an
implementation of the well-known clock api in include/clk/clk.h.
Platforms may define their own hardware-specific clock structure and
their own clock operation callbacks, so long as it wraps an instance of
struct clk_hw.
See Documentation/clk.txt for more details.
This patch is based on the work of Jeremy Kerr, which in turn was based
on the work of Ben Herrenschmidt.
+
+/**
+ * struct clk_hw - handle for traversing from a struct clk to its
corresponding
+ * hardware-specific structure. struct clk_hw should be declared within
struct
+ * clk_foo and then referenced by the struct clk instance that uses struct
+ * clk_foo's clk_ops
+ *
+ * clk: pointer to the struct clk instance that points back to this struct
+ * clk_hw instance
+ */
+struct clk_hw {
+ struct clk *clk;
+};
The reason for doing this is that struct clk should be an opaque cookie
for both drivers and implementers of clocks. I recently had the idea whether
the roles of these two structs could be swapped. So instead of the above we
could do:
struct clk {
struct clk_hw *hw;
}
The effect would be the same, but this version would make the struct clk
pointer known at compile time thus making it possible for static
initializers to specify their parent clock.
I just saw that clk_register now takes a parent name array, that may
make this change unnecessary anyway.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure
On Mon, Oct 17, 2011 at 06:53:03PM +0800, Richard Zhao wrote:
On Mon, Oct 17, 2011 at 10:20:28AM +0100, Mark Brown wrote:
On Mon, Oct 17, 2011 at 04:48:52PM +0800, Richard Zhao wrote:
For example, devices that possible access to on-chip RAM, depend on OCRAM
clock.
On imx53, VPU depends on OCRAM clock, even when VPU does not use OCRAM.
So if the VPU depends on OCRAM the VPU should be enabling the OCRAM
clock. The function of a given clock isn't terribly relevant, and
certainly grouping clocks together doesn't seem to be the obvious
solution from what you've said
VPU don't know OCRAM clk, it's SoC specific. I know it's clock function
replationship. But it's the only better place to refect the dependency
in clock tree. Another dependency example, from SoC bus topology, some bus
clk always depends on bus switch/hub.
- if the driver doesn't know about the
clock it seems like the core ought to be enabling it transparently
rather than gluing it together with some other random clock.
If you mean clk core here, then we need things like below:
struct clk_hw {
struct clk *clk;
struct dependency {
struct clk_hw *clks;
int count;
};
};
Though Mike does not like to add things in clk_hw, but it's the only place
to put common things of clk drivers.
It's not a problem to associate multiple clocks to a device, we can do
this now already. What a driver can't do now is give-me-all-clocks-I-need(dev),
but this problem should not be solved at clock core level but at the
clkdev level.
The fact is that the different clocks for a device are really different
clocks. A dumb driver may want to request/enable all relevant clocks at
once while a more sophisticated driver may want to enable the clock for
accessing registers in the probe function and a baud clock on device
open time.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure
On Fri, Oct 14, 2011 at 06:32:33PM +0800, Richard Zhao wrote:
On Fri, Oct 14, 2011 at 11:05:04AM +0100, Mark Brown wrote:
On Fri, Oct 14, 2011 at 04:10:26PM +0800, Richard Zhao wrote:
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
snip essentially Mike's entire mail - *please* delete irrelevant quotes
from your replies, it makes it very much easier to find the new text in
your mail and is much more friendly to people reading mail on mobile
devices.
I snip not enough? sorry for that. I'll be carefull.
+static int __clk_enable(struct clk *clk)
+{
Could you expose __clk_enable/__clk_disable? I find it hard to implement
clk group. clk group means, when a major clk enable/disable, it want a set
of other clks enable/disable accordingly.
Shouldn't this be something the core is implementing? I'd strongly
expect that the clock drivers are relatively dumb and delegate all the
decision making to the core API. Otherwise it's going to be hard for
the core to implement any logic that involves working with more than one
clock like rate change notification, or guarantee that driver requests
made through the API are satisfied, as the state of the clocks will be
changing underneath it.
From my point of view, the first step of generic clk can be, easy to adopt
features of clocks in current mainline git.
Back to the clk group, I have a patch based on Sascha's work.
http://git.linaro.org/gitweb?p=people/riczhao/linux-2.6.git;a=shortlog;h=refs/heads/imx-clk
I thought further about this and a clock group is not something we want
to have at all. Clocks are supposed to be arranged in a tree and
grouping clocks together violates this which leads to problems.
This grouping should be done at driver level, so when a driver needs
more than one clock it should request them all, maybe with a clk_get_all
helper function.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 4/7] clk: Add simple gated clock
On Wed, Oct 12, 2011 at 07:59:19AM -0700, Turquette, Mike wrote:
On Tue, Oct 11, 2011 at 11:46 PM, Richard Zhao
richard.z...@freescale.com wrote:
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote:
From: Jeremy Kerr jeremy.k...@canonical.com
Signed-off-by: Jeremy Kerr jeremy.k...@canonical.com
Signed-off-by: Mark Brown broo...@opensource.wolfsonmicro.com
Signed-off-by: Jamie Iles ja...@jamieiles.com
Signed-off-by: Mike Turquette mturque...@ti.com
---
Changes since v1:
Add copyright header
Fold in Jamie's patch for set-to-disable clks
Use BIT macro instead of shift
drivers/clk/Kconfig | 4 ++
drivers/clk/Makefile | 1 +
drivers/clk/clk-gate.c | 78
include/linux/clk.h | 13
4 files changed, 96 insertions(+), 0 deletions(-)
create mode 100644 drivers/clk/clk-gate.c
I feel hard to tell the tree the clk parent, at register/init time. For the
simple gate clk, the only way is to set .get_parent. But normally, for clk
without any divider we set .get_parent to NULL. Maybe we can put .parent to
struct clk_hw?
For non-mux clocks, whose parent is *always* going to be the same, you
should create a duplicate .parent in the clk_hw_* structure and then
have .get_parent return clk_hw_*-parent.
Maybe I do not understand what you mean here, but I think there is
something missing in the gate.
This is analogous to the way clk_hw_fixed returns clk_hw_fixed-rate
when .recalc is called on it.
+
+static unsigned long clk_gate_get_rate(struct clk_hw *clk)
+{
+ return clk_get_rate(clk_get_parent(clk-clk));
+}
clk_get_parent goes down to clk_gate_set_enable_ops.get_parent below...
+
+
+struct clk_hw_ops clk_gate_set_enable_ops = {
+ .recalc_rate = clk_gate_get_rate,
+ .enable = clk_gate_enable_set,
+ .disable = clk_gate_disable_clear,
+};
...but this does not have a get_parent pointer, so clk_get_parent()
for a gate always returns NULL which means that a gate never has
a valid rate.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/4 v4] drivers: create a pin control subsystem
On Thu, Aug 25, 2011 at 12:12:59PM +0200, Linus Walleij wrote:
On Wed, Aug 24, 2011 at 8:29 PM, Stephen Warren swar...@nvidia.com wrote:
Linus Walleij wrote at Friday, August 19, 2011 3:54 AM:
This creates a subsystem for handling of pin control devices.
These are devices that control different aspects of package
pins.
Sorry to keep harping on about this, but I'm still not convinced the data
model is correct.
Don't feel sorry! I'm getting very much done and much important
research and thinking is happening thanks to you valuable feedback.
Keep doing this!
What I notice is that the review comments have changed focus
from the earlier contention point about having multiple functions
per map entry to something else, which to me seems like it
comes from device tree work, and is about describing how each
and every pin is wired to its function. Does this correspond to
your recent pinmux experience?
Here are the two possible models:
Model 1 (as implemented in the pin control subsystem):
* Each pinmux chip defines a number of functions.
* Each function describes what functionality is mux'd onto the pins.
* Each function describes which pins are affected by the function.
This is correct.
Result:
If there are a couple of controllers that can each be mux'd out two
places, you get four functions:
Function i2c0-0: Controller i2c0 is mux'd onto pins 0 and 1
Function i2c0-1: Controller i2c0 is mux'd onto pins 2 and 3
Function spi0-0: Controller spi0 is mux'd onto pins 0 and 1
Function spi0-1: Controller spi0 is mux'd onto pins 4 and 5
Yes, if and only if the pinmux driver find it useful to expose
these two ports on the two sets of pins each.
For example: there really *is* a bus soldered to pin {0,1}
and another bus soldered to pin {2,3}, and each has devices
on it. But I have only one single i2c controller i2c0.
If pins {2,3} were not soldered or used for something else
it doesn't make sense for the pin controller to expose
two functions like this.
So this is dependent on platform/board data. We need
to know how the chip has been deployed in this very
machine to know what functions to expose.
Model 2 (what I'd like to see)
* Each pinmux chip defines a number of functions.
* Each function describes the functionality that can be mux'd out.
* Each pinmux chip defines a number of pins.
* Each pinmux chip defines which functions are legal on which pins.
Result:
With the same controllers and pins above, you get:
Function i2c0
Function spi0
Pins 0, 1, 2, 3, 4, 5
Pins 0, 1 accept functions i2c0, spi0
Pins 2, 3 accept functions i2c0
Pins 4, 5 accept functions spi0
We'd still have a single controller-wide namespace for functions, it's
just that functions are something you mux onto pins, not the combination
of mux'd functionality and the pins it's been mux'd onto.
I think I understand this. Maybe.
I read it like this:
Legend:
1..* = one to many
1..1 = one to one
- Pins has a 1..* relation to functions
- Functions in general have a 1..* relation to pins,
- Device drivers in general have a 1..* relation to
functions
- Functions with 1..1 relation to pins and device drives
with a 1..1 relation to functions is not the norm
So this is radically different in that it requires the pin control
system to assume basically that any one pin can be used for
any one function.
I refer to this as a phone-exchange model, like any one pin
can map to any one function, like any phone can call any
other phone.
The current model is not based on that assumption at
all. The current model is derived from some available
pinmux controllers and described below.
* I believe this model is much more directly maps to the way most HW
works; there's a register for each pin where you write a value to select
which functionality to mux onto that pin. Assuming that's true, using
this data model might lead to simpler pinmux chip implementations; they'd
require fewer mapping tables.
I understand this statement as you assume al pinmuxes
are phone-exachange-type, where any pin can be tied to
any function at runtime. So for example the CLK pin of spi0
can be muxed onto any pin basically, not a predetermined
set of pins.
I think that data model does not take into account the fact
that all or most pinmuxes are inherently restricted and not at
all that open-ended. That model would impose a
phone-exchange type of data model on hardware that is
much more limited.
So the data model I'm assuming is:
- Pins has a 1..* relation to functions
- Functions in general have a 1..1 relation to pins,
- Device drivers in general have a 1..1 relation to
functions
- Functions with 1..* relation to pins is uncommon
as is 1..* realtions between device drivers and
functions.
The latter is the crucial point where I think we have
different assumptions.
If it holds, it leads to the
Re: [PATCH 1/4 v4] drivers: create a pin control subsystem
On Thu, Aug 25, 2011 at 01:58:12PM +0200, Linus Walleij wrote: On Thu, Aug 25, 2011 at 1:04 PM, Sascha Hauer s.ha...@pengutronix.de wrote: Not really. UART2_CTS can't be routed to arbitrary pads, but it can be routed to more than one pad: #define _MX51_PAD_EIM_D16__UART2_CTS IOMUX_PAD(0x3f0, 0x5c, 3, 0x, 0, 0) #define _MX51_PAD_EIM_D25__UART2_CTS IOMUX_PAD(0x414, 0x80, 4, 0x, 0, 0) #define _MX51_PAD_USBH1_DATA0__UART2_CTS IOMUX_PAD(0x688, 0x288, 1, 0x, 0, 0) Aha! Is it typically a few pads like this, say 2,3,4 alternatives, sometimes just one? So the actual relation is not 1..1 nor 1...* but 1..a few? Yes. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/2] ARM: IMX5: cpuidle driver
On Thu, Feb 17, 2011 at 09:18:11AM +0100, Yong Shen wrote: + return 0; +} + +late_initcall(imx_cpuidle_init); We have a late_initcall here which needs to be protected from other cpus. On the other hand we depend on board code calling imx_cpuidle_board_params() before this initcall. I think the board code should call a imx_cpuidle_init(struct imx_cpuidle_params *cpuidle_params) instead which makes the flow of execution more clear. imx_cpuidle_init can not be called directly in board code, since it is too early to register cpuidle driver and device which depend on some other system resource. I see. Maybe we should make this a platform driver then like for example davinci does. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/2] ARM: IMX5: cpuidle driver
On Fri, Feb 11, 2011 at 10:36:12AM +0100, yong.s...@linaro.org wrote:
From: Yong Shen yong.s...@freescale.com
implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board
related code.
Signed-off-by: Yong Shen yong.s...@freescale.com
---
arch/arm/mach-mx5/Makefile |1 +
arch/arm/mach-mx5/cpuidle.c | 113
+++
arch/arm/mach-mx5/cpuidle.h | 26 ++
3 files changed, 140 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-mx5/cpuidle.c
create mode 100644 arch/arm/mach-mx5/cpuidle.h
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index 0d43be9..12239e0 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
@@ -7,6 +7,7 @@ obj-y := cpu.o mm.o clock-mx51-mx53.o devices.o
obj-$(CONFIG_SOC_IMX50) += mm-mx50.o
obj-$(CONFIG_CPU_FREQ_IMX)+= cpu_op-mx51.o
+obj-$(CONFIG_CPU_IDLE) += cpuidle.o
obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o
obj-$(CONFIG_MACH_MX53_EVK) += board-mx53_evk.o
diff --git a/arch/arm/mach-mx5/cpuidle.c b/arch/arm/mach-mx5/cpuidle.c
new file mode 100644
index 000..9d77c47
--- /dev/null
+++ b/arch/arm/mach-mx5/cpuidle.c
@@ -0,0 +1,113 @@
+/*
+ * arch/arm/mach-mx5/cpuidle.c
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed as is without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include linux/io.h
+#include linux/cpuidle.h
+#include asm/proc-fns.h
+#include mach/hardware.h
+#include cpuidle.h
+#include crm_regs.h
+
+static struct imx_cpuidle_params *imx_cpuidle_params;
+void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
+{
+ imx_cpuidle_params = cpuidle_params;
+}
+
+extern int tzic_enable_wake(int is_idle);
+static int imx_enter_idle(struct cpuidle_device *dev,
+struct cpuidle_state *state)
+{
+ struct timeval before, after;
+ int idle_time;
+ u32 plat_lpc, arm_srpgcr, ccm_clpcr;
+ u32 empgc0, empgc1;
+
+ local_irq_disable();
+ do_gettimeofday(before);
+
+ plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC)
+ ~(MXC_CORTEXA8_PLAT_LPC_DSM);
One thing that strikes me here is the fact that this code can probably
run on i.MX53 aswell, right? It's only that these registers have
different addresses on i.MX53. The MXC_ prefix is therefore not a good
idea. Switching this to MX51_ and having an additional MX53_ register
leads to code duplication. This shows that it's a bad idea to code
fixed addresses in the code. We should go for base + offset instead
so that this code will have a better start on i.MX53. This of course
needs changes in the current crm_regs.h and probably in the i.MX51/53
clock code.
+ ccm_clpcr = __raw_readl(MXC_CCM_CLPCR) ~(MXC_CCM_CLPCR_LPM_MASK);
+ arm_srpgcr = __raw_readl(MXC_SRPG_ARM_SRPGCR) ~(MXC_SRPGCR_PCR);
+ empgc0 = __raw_readl(MXC_SRPG_EMPGC0_SRPGCR) ~(MXC_SRPGCR_PCR);
+ empgc1 = __raw_readl(MXC_SRPG_EMPGC1_SRPGCR) ~(MXC_SRPGCR_PCR);
+
+ if (state == dev-states[WAIT_CLK_ON])
+ ;
+ else if (state == dev-states[WAIT_CLK_OFF])
+ ccm_clpcr |= (0x1 MXC_CCM_CLPCR_LPM_OFFSET);
+ else if (state == dev-states[WAIT_CLK_OFF_POWER_OFF]) {
+ /* Wait unclocked, power off */
+ plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM
+ | MXC_CORTEXA8_PLAT_LPC_DBG_DSM;
+ arm_srpgcr |= MXC_SRPGCR_PCR;
+ ccm_clpcr |= (0x1 MXC_CCM_CLPCR_LPM_OFFSET);
+ ccm_clpcr = ~MXC_CCM_CLPCR_VSTBY;
+ ccm_clpcr = ~MXC_CCM_CLPCR_SBYOS;
+ if (tzic_enable_wake(1) != 0) {
+ local_irq_enable();
+ return 0;
+ }
+ }
+
+ __raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);
+ __raw_writel(ccm_clpcr, MXC_CCM_CLPCR);
+ __raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);
+
+ cpu_do_idle();
+
+ do_gettimeofday(after);
+ local_irq_enable();
+ idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
+ (after.tv_usec - before.tv_usec);
+ return idle_time;
+}
+
+static struct cpuidle_driver imx_cpuidle_driver = {
+ .name = imx_idle,
+ .owner =THIS_MODULE,
+};
+
+static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
+
+static int __init imx_cpuidle_init(void)
+{
+ struct cpuidle_device *device;
+ int i;
+
+ if (imx_cpuidle_params == NULL)
+ return -ENODEV;
+
+ cpuidle_register_driver(imx_cpuidle_driver);
+
+ device = per_cpu(imx_cpuidle_device, smp_processor_id());
+ device-state_count = IMX_MAX_CPUIDLE_STATE;
+
+ for (i = 0; i IMX_MAX_CPUIDLE_STATE i CPUIDLE_STATE_MAX; i++) {
+ device-states[i].enter
Re: [PATCH v2 1/2] ARM: IMX5: cpuidle driver
On Fri, Feb 11, 2011 at 10:36:12AM +0100, yong.s...@linaro.org wrote:
From: Yong Shen yong.s...@freescale.com
implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board
related code.
Signed-off-by: Yong Shen yong.s...@freescale.com
---
arch/arm/mach-mx5/Makefile |1 +
arch/arm/mach-mx5/cpuidle.c | 113
+++
arch/arm/mach-mx5/cpuidle.h | 26 ++
3 files changed, 140 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-mx5/cpuidle.c
create mode 100644 arch/arm/mach-mx5/cpuidle.h
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index 0d43be9..12239e0 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
@@ -7,6 +7,7 @@ obj-y := cpu.o mm.o clock-mx51-mx53.o devices.o
obj-$(CONFIG_SOC_IMX50) += mm-mx50.o
obj-$(CONFIG_CPU_FREQ_IMX)+= cpu_op-mx51.o
+obj-$(CONFIG_CPU_IDLE) += cpuidle.o
obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o
obj-$(CONFIG_MACH_MX53_EVK) += board-mx53_evk.o
diff --git a/arch/arm/mach-mx5/cpuidle.c b/arch/arm/mach-mx5/cpuidle.c
new file mode 100644
index 000..9d77c47
--- /dev/null
+++ b/arch/arm/mach-mx5/cpuidle.c
@@ -0,0 +1,113 @@
+/*
+ * arch/arm/mach-mx5/cpuidle.c
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed as is without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include linux/io.h
+#include linux/cpuidle.h
+#include asm/proc-fns.h
+#include mach/hardware.h
+#include cpuidle.h
+#include crm_regs.h
+
+static struct imx_cpuidle_params *imx_cpuidle_params;
+void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
+{
+ imx_cpuidle_params = cpuidle_params;
+}
+
+extern int tzic_enable_wake(int is_idle);
Please put this into a header file.
+static int imx_enter_idle(struct cpuidle_device *dev,
+struct cpuidle_state *state)
+{
+ struct timeval before, after;
+ int idle_time;
+ u32 plat_lpc, arm_srpgcr, ccm_clpcr;
+ u32 empgc0, empgc1;
+
+ local_irq_disable();
+ do_gettimeofday(before);
+
+ plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC)
+ ~(MXC_CORTEXA8_PLAT_LPC_DSM);
+ ccm_clpcr = __raw_readl(MXC_CCM_CLPCR) ~(MXC_CCM_CLPCR_LPM_MASK);
+ arm_srpgcr = __raw_readl(MXC_SRPG_ARM_SRPGCR) ~(MXC_SRPGCR_PCR);
+ empgc0 = __raw_readl(MXC_SRPG_EMPGC0_SRPGCR) ~(MXC_SRPGCR_PCR);
+ empgc1 = __raw_readl(MXC_SRPG_EMPGC1_SRPGCR) ~(MXC_SRPGCR_PCR);
+
+ if (state == dev-states[WAIT_CLK_ON])
+ ;
An if without code? This looks strange.
+ else if (state == dev-states[WAIT_CLK_OFF])
+ ccm_clpcr |= (0x1 MXC_CCM_CLPCR_LPM_OFFSET);
+ else if (state == dev-states[WAIT_CLK_OFF_POWER_OFF]) {
+ /* Wait unclocked, power off */
+ plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM
+ | MXC_CORTEXA8_PLAT_LPC_DBG_DSM;
+ arm_srpgcr |= MXC_SRPGCR_PCR;
+ ccm_clpcr |= (0x1 MXC_CCM_CLPCR_LPM_OFFSET);
+ ccm_clpcr = ~MXC_CCM_CLPCR_VSTBY;
+ ccm_clpcr = ~MXC_CCM_CLPCR_SBYOS;
+ if (tzic_enable_wake(1) != 0) {
+ local_irq_enable();
+ return 0;
+ }
+ }
+
+ __raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);
+ __raw_writel(ccm_clpcr, MXC_CCM_CLPCR);
+ __raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);
+
+ cpu_do_idle();
+
+ do_gettimeofday(after);
+ local_irq_enable();
+ idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
+ (after.tv_usec - before.tv_usec);
+ return idle_time;
+}
+
+static struct cpuidle_driver imx_cpuidle_driver = {
+ .name = imx_idle,
+ .owner =THIS_MODULE,
+};
+
+static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
+
+static int __init imx_cpuidle_init(void)
+{
+ struct cpuidle_device *device;
+ int i;
+
+ if (imx_cpuidle_params == NULL)
+ return -ENODEV;
+
+ cpuidle_register_driver(imx_cpuidle_driver);
+
+ device = per_cpu(imx_cpuidle_device, smp_processor_id());
+ device-state_count = IMX_MAX_CPUIDLE_STATE;
+
+ for (i = 0; i IMX_MAX_CPUIDLE_STATE i CPUIDLE_STATE_MAX; i++) {
+ device-states[i].enter = imx_enter_idle;
+ device-states[i].exit_latency = imx_cpuidle_params[i].latency;
+ device-states[i].flags = CPUIDLE_FLAG_TIME_VALID;
+ }
+
+ strcpy(device-states[WAIT_CLK_ON].name, WFI 0);
+ strcpy(device-states[WAIT_CLK_ON].desc, Wait with clock on);
+ strcpy(device-states[WAIT_CLK_OFF].name, WFI 1);
+ strcpy(device-states[WAIT_CLK_OFF].desc, Wait with clock off);
+ strcpy(device-states[WAIT_CLK_OFF_POWER_OFF].name, WFI
Re: [PATCHv2] make mc13783 regulator code generic
Hi Yong, On Wed, Dec 01, 2010 at 03:15:55PM +0800, Yong Shen wrote: Hi there, This is the v2 with some changes according to comments from v1. There will be few patches coming out after this one, for mc13892 regulator to share some code with mc13783. Still, cause the firewall problem, I send out patch this way. Please give comments inline and use attached patch for testing. This patch definitely goes into the right direction. Some comments inline. Yong From b3451070f4665c90f11e600a8722f86b68437ded Mon Sep 17 00:00:00 2001 From: Yong Shen yong.s...@linaro.org Date: Tue, 30 Nov 2010 14:11:34 +0800 Subject: [PATCH] make mc13783 regulator code generic move some common functions and micros of mc13783 regulaor driver to a seperate file, which makes it possible for mc13892 to share code. Signed-off-by: Yong Shen yong.s...@linaro.org --- drivers/regulator/Kconfig |4 + drivers/regulator/Makefile |1 + drivers/regulator/mc13783-regulator.c | 325 drivers/regulator/mc13xxx-regulator-core.c | 239 include/linux/mfd/mc13783.h| 67 +++--- include/linux/regulator/mc13xxx.h | 101 + 6 files changed, 425 insertions(+), 312 deletions(-) create mode 100644 drivers/regulator/mc13xxx-regulator-core.c create mode 100644 include/linux/regulator/mc13xxx.h diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index dd30e88..63c2bdd 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -186,9 +186,13 @@ config REGULATOR_PCAP This driver provides support for the voltage regulators of the PCAP2 PMIC. +config REGULATOR_MC13XXX_CORE + tristate Support regulators on Freescale MC13xxx PMIC + This does not need a prompt. Selecting only this option does not make sense and it will be selected by the mc13783/mc13892 code anyway. config REGULATOR_MC13783 tristate Support regulators on Freescale MC13783 PMIC depends on MFD_MC13783 + select REGULATOR_MC13XXX_CORE help Say y here to support the regulators found on the Freescale MC13783 PMIC. diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index bff8157..11876be 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_REGULATOR_DA903X) += da903x.o obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o obj-$(CONFIG_REGULATOR_PCAP) += pcap-regulator.o obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o +obj-$(CONFIG_REGULATOR_MC13XXX_CORE) += mc13xxx-regulator-core.o obj-$(CONFIG_REGULATOR_AB3100) += ab3100.o obj-$(CONFIG_REGULATOR_TPS65023) += tps65023-regulator.o [snip] diff --git a/include/linux/mfd/mc13783.h b/include/linux/mfd/mc13783.h index b4c741e..7d0f3d6 100644 --- a/include/linux/mfd/mc13783.h +++ b/include/linux/mfd/mc13783.h @@ -1,4 +1,5 @@ /* + * Copyright 2010 Yong Shen yong.s...@linaro.org * Copyright 2009-2010 Pengutronix * Uwe Kleine-Koenig u.kleine-koe...@pengutronix.de * @@ -122,39 +123,39 @@ int mc13783_adc_do_conversion(struct mc13783 *mc13783, unsigned int mode, unsigned int channel, unsigned int *sample); -#define MC13783_SW_SW1A 0 -#define MC13783_SW_SW1B 1 -#define MC13783_SW_SW2A 2 -#define MC13783_SW_SW2B 3 -#define MC13783_SW_SW3 4 -#define MC13783_SW_PLL 5 -#define MC13783_REGU_VAUDIO 6 -#define MC13783_REGU_VIOHI 7 -#define MC13783_REGU_VIOLO 8 -#define MC13783_REGU_VDIG 9 -#define MC13783_REGU_VGEN 10 -#define MC13783_REGU_VRFDIG 11 -#define MC13783_REGU_VRFREF 12 -#define MC13783_REGU_VRFCP 13 -#define MC13783_REGU_VSIM 14 -#define MC13783_REGU_VESIM 15 -#define MC13783_REGU_VCAM 16 -#define MC13783_REGU_VRFBG 17 -#define MC13783_REGU_VVIB 18 -#define MC13783_REGU_VRF1 19 -#define MC13783_REGU_VRF2 20 -#define MC13783_REGU_VMMC1 21 -#define MC13783_REGU_VMMC2 22 -#define MC13783_REGU_GPO1 23 -#define MC13783_REGU_GPO2 24 -#define MC13783_REGU_GPO3 25 -#define MC13783_REGU_GPO4 26 -#define MC13783_REGU_V1 27 -#define MC13783_REGU_V2 28 -#define MC13783_REGU_V3 29 -#define MC13783_REGU_V4 30 -#define MC13783_REGU_PWGT1SPI 31 -#define MC13783_REGU_PWGT2SPI 32 These have users. If you really want to change them you must change the users aswell. Also, I would suggest an additional patch for this. Remember, one topic per patch. Renaming things is a topic that can easily be split out. +#define MC13783_REG_SW1A0 +#define
Re: [PATCHv3] cpufreq for freescale mx51
On Tue, Oct 19, 2010 at 05:28:51PM +0800, Yong Shen wrote:
+#include linux/kernel.h
+
+static struct cpu_op mx51_cpu_op[] = {
+ {
+ .cpu_rate = 16000,},
+ {
+ .cpu_rate = 8,},
+};
Why did you remove the values between 800MHz and 160MHz? 400MHz and
200Mhz should work also, no?
It proved that those operating points don't work well.
ok
+static struct cpu_op *cpu_op_tbl;
+
+static int set_cpu_freq(int freq)
+{
+ int ret = 0;
+ int org_cpu_rate;
+
+ org_cpu_rate = clk_get_rate(cpu_clk);
+ if (org_cpu_rate == freq)
+ return ret;
You already checked this in mxc_set_target. Once is enough.
Well, this fucntion is not only used in mxc_set_target, so it is safer to
still keep checking here.
Then you can skip the check in the calling function.
+
+static int __init mxc_cpufreq_init(struct cpufreq_policy *policy)
+{
+ int ret;
+ int i;
+
+ printk(KERN_INFO i.MXC CPU frequency driver\n);
+
+ if (policy-cpu != 0)
+ return -EINVAL;
+
+ cpu_clk = clk_get(NULL, cpu_clk);
+ if (IS_ERR(cpu_clk)) {
+ printk(KERN_ERR %s: failed to get cpu clock\n, __func__);
+ return PTR_ERR(cpu_clk);
+ }
+
+ cpu_op_tbl = get_cpu_op(cpu_op_nr);
This will crash every board except the babbage board which happens to
set this function pointer.
Add a checking here should avoid that.
indeed
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCHv2] cpufreq for freescale mx51
Hi Yong,
On Mon, Oct 18, 2010 at 01:43:43PM +0800, Yong Shen wrote:
Hi Sascha,
Thanks for your thorough review. I have two feedbacks to your commends.
Sorry for delayed response, cause I had a hard time due to my computer crash
and data loss.
diff --git a/arch/arm/mach-mx5/cpu.c b/arch/arm/mach-mx5/cpu.c
index 2d37785..83add9c 100644
--- a/arch/arm/mach-mx5/cpu.c
+++ b/arch/arm/mach-mx5/cpu.c
@@ -22,6 +22,8 @@ static int cpu_silicon_rev = -1;
#define SI_REV 0x48
+struct cpu_wp *(*get_cpu_wp)(int *wp);
+
This is not needed.
This is needed, otherwise it does not pass compile.
This hunk is the only change to arch/arm/mach-mx5/cpu.c and get_cpu_wp
is introduced with this patch, so how can this break compilation?
Also, you should move this to a header file. Otherwise you risk of
having multiple (and possibly different) declarations of the same
function which can lead to hard to find bugs.
+ return ret;
+ }
+
+ cpufreq_frequency_table_get_attr(imx_freq_table, policy-cpu);
+ return 0;
+}
+
+static int mxc_cpufreq_exit(struct cpufreq_policy *policy)
+{
+ cpufreq_frequency_table_put_attr(policy-cpu);
+
+ /* Reset CPU to 665MHz */
+ set_cpu_freq(arm_normal_clk);
arm_normal_clk is initialized to cpu_freq_khz_max * 1000 and never touched
again. It would be clearer here to remove arm_normal_clk and write
cpu_freq_khz_max * 1000 directly here. Then you can also remove the
comment which is wrong for most i.MXs, even for the i.MX51.
Actually, this is arm_normal_clk is touched later in mxc_cpufreq_exit()
fuction.
It's *read* but not changed. That's why I suggested to just remove
arm_normal_clk and instead do a set_cpu_freq(cpu_freq_khz_max * 1000) in
mxc_cpufreq_exit.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCHv3] cpufreq for freescale mx51
On Mon, Oct 18, 2010 at 04:21:45PM +0800, yong.s...@linaro.org wrote:
From: Yong Shen yong.s...@linaro.org
the operating points are tested on babbage 3.0
Signed-off-by: Yong Shen yong.s...@linaro.org
---
arch/arm/Kconfig |6 +
arch/arm/mach-mx5/Kconfig |1 +
arch/arm/mach-mx5/Makefile |1 +
arch/arm/mach-mx5/board-mx51_babbage.c | 12 ++-
arch/arm/mach-mx5/clock-mx51.c | 22 +++-
arch/arm/mach-mx5/cpu.c|2 +
arch/arm/mach-mx5/cpu_op-mx51.c| 29 +
arch/arm/mach-mx5/cpu_op-mx51.h| 14 +++
arch/arm/plat-mxc/Makefile |1 +
arch/arm/plat-mxc/cpufreq.c| 202
arch/arm/plat-mxc/include/mach/mxc.h | 20 +++-
11 files changed, 306 insertions(+), 4 deletions(-)
create mode 100644 arch/arm/mach-mx5/cpu_op-mx51.c
create mode 100644 arch/arm/mach-mx5/cpu_op-mx51.h
create mode 100644 arch/arm/plat-mxc/cpufreq.c
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index e4ea9cb..d9ad605 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1692,6 +1692,12 @@ if ARCH_HAS_CPUFREQ
source drivers/cpufreq/Kconfig
+config CPU_FREQ_IMX
+ tristate CPUfreq driver for i.MX CPUs
+ depends on ARCH_MXC CPU_FREQ
+ help
+ This enables the CPUfreq driver for i.MX CPUs.
+
config CPU_FREQ_SA1100
bool
diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
index 0848db5..7a621b4 100644
--- a/arch/arm/mach-mx5/Kconfig
+++ b/arch/arm/mach-mx5/Kconfig
@@ -5,6 +5,7 @@ config ARCH_MX51
default y
select MXC_TZIC
select ARCH_MXC_IOMUX_V3
+ select ARCH_HAS_CPUFREQ
comment MX5 platforms:
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index 86c66e7..2fadac5 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
@@ -5,6 +5,7 @@
# Object file lists.
obj-y := cpu.o mm.o clock-mx51.o devices.o
+obj-$(CONFIG_CPU_FREQ_IMX)+= cpu_op-mx51.o
obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o
obj-$(CONFIG_MACH_EUKREA_CPUIMX51) += board-cpuimx51.o
diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c
b/arch/arm/mach-mx5/board-mx51_babbage.c
index 6e384d9..74c796f 100644
--- a/arch/arm/mach-mx5/board-mx51_babbage.c
+++ b/arch/arm/mach-mx5/board-mx51_babbage.c
@@ -1,5 +1,5 @@
/*
- * Copyright 2009 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved.
* Copyright (C) 2009-2010 Amit Kucheria amit.kuche...@canonical.com
*
* The code contained herein is licensed under the GNU General Public
@@ -32,6 +32,7 @@
#include asm/mach/time.h
#include devices.h
+#include cpu_op-mx51.h
#define BABBAGE_USB_HUB_RESET(0*32 + 7) /* GPIO_1_7 */
#define BABBAGE_USBH1_STP(0*32 + 27) /* GPIO_1_27 */
@@ -279,8 +280,17 @@ static struct sys_timer mxc_timer = {
.init = mx51_babbage_timer_init,
};
+static void __init fixup_mxc_board(struct machine_desc *desc, struct tag
*tags,
+char **cmdline, struct meminfo *mi)
+{
+#if defined(CONFIG_CPU_FREQ_IMX)
+ get_cpu_op = mx51_get_cpu_op;
+#endif
+}
+
Why has this to live in .fixup? .init_machine should be fine here.
MACHINE_START(MX51_BABBAGE, Freescale MX51 Babbage Board)
/* Maintainer: Amit Kucheria amit.kuche...@canonical.com */
+ .fixup = fixup_mxc_board,
.phys_io = MX51_AIPS1_BASE_ADDR,
.io_pg_offst = ((MX51_AIPS1_BASE_ADDR_VIRT) 18) 0xfffc,
.boot_params = PHYS_OFFSET + 0x100,
diff --git a/arch/arm/mach-mx5/clock-mx51.c b/arch/arm/mach-mx5/clock-mx51.c
index 6af69de..f89d8a0 100644
--- a/arch/arm/mach-mx5/clock-mx51.c
+++ b/arch/arm/mach-mx5/clock-mx51.c
@@ -330,7 +330,7 @@ static int _clk_lp_apm_set_parent(struct clk *clk, struct
clk *parent)
return 0;
}
-static unsigned long clk_arm_get_rate(struct clk *clk)
+static unsigned long clk_cpu_get_rate(struct clk *clk)
{
u32 cacrr, div;
unsigned long parent_rate;
@@ -342,6 +342,22 @@ static unsigned long clk_arm_get_rate(struct clk *clk)
return parent_rate / div;
}
+static int clk_cpu_set_rate(struct clk *clk, unsigned long rate)
+{
+ u32 reg, cpu_podf;
+ unsigned long parent_rate;
+
+ parent_rate = clk_get_rate(clk-parent);
+ cpu_podf = parent_rate / rate - 1;
+ /* use post divider to change freq */
+ reg = __raw_readl(MXC_CCM_CACRR);
+ reg = ~MXC_CCM_CACRR_ARM_PODF_MASK;
+ reg |= cpu_podf MXC_CCM_CACRR_ARM_PODF_OFFSET;
+ __raw_writel(reg, MXC_CCM_CACRR);
+
+ return 0;
+}
+
static int _clk_periph_apm_set_parent(struct clk *clk, struct clk *parent)
{
u32 reg, mux;
@@ -693,7 +709,8 @@ static struct clk periph_apm_clk = {
static
Re: [PATCHv2] cpufreq for freescale mx51
On Mon, Oct 18, 2010 at 05:08:14PM +0800, Yong Shen wrote: Hi Sascha, On Mon, Oct 18, 2010 at 4:31 PM, Sascha Hauer s.ha...@pengutronix.dewrote: Hi Yong, On Mon, Oct 18, 2010 at 01:43:43PM +0800, Yong Shen wrote: Hi Sascha, Thanks for your thorough review. I have two feedbacks to your commends. Sorry for delayed response, cause I had a hard time due to my computer crash and data loss. diff --git a/arch/arm/mach-mx5/cpu.c b/arch/arm/mach-mx5/cpu.c index 2d37785..83add9c 100644 --- a/arch/arm/mach-mx5/cpu.c +++ b/arch/arm/mach-mx5/cpu.c @@ -22,6 +22,8 @@ static int cpu_silicon_rev = -1; #define SI_REV 0x48 +struct cpu_wp *(*get_cpu_wp)(int *wp); + This is not needed. This is needed, otherwise it does not pass compile. This hunk is the only change to arch/arm/mach-mx5/cpu.c and get_cpu_wp is introduced with this patch, so how can this break compilation? Also, you should move this to a header file. Otherwise you risk of having multiple (and possibly different) declarations of the same function which can lead to hard to find bugs. IMHO, get_cpu_wp is definition rather than a declaration and without it there will be errors in link phase. its declaration is in arch/arm/plat-mxc/include/mach/mxc.h. Of course, you are right, my bad. Isn't this function common to al i.MXs? In this case it should be somewhere in plat-mxc. Anyway, it seems very odd to me to provide a global function pointer which gets overwritten by the boards. For me it is more logical to provide a mxc_register_workpoints() function along with a mxc_for_each_workpoint() iterator. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] cpufreq for freescale mx51
On Thu, Oct 07, 2010 at 10:40:44AM +0300, Amit Kucheria wrote: On 10 Oct 07, Yong Shen wrote: +static struct cpufreq_frequency_table imx_freq_table[4]; Three frequencies should be enough for everyone, right? This should be dynamically allocated like in other cpufreq drivers. Yes, we only support 3 work points, which is for sure. Actually, we only support 2 points on most mx51 chips. I supposed it is ok to use static array here. This can become a common cpufreq driver for all i.MX platforms. We don't know how many work points will be supported in future versions of the silicon. That is why a static array is not ok. I think Sascha was being ironic when he said Three frequencies should be enough for everyone, right? :) Yes, the old 640k-is-enough-for-anyone joke. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] cpufreq for freescale mx51
On Thu, Oct 07, 2010 at 11:36:07AM +0800, Yong Shen wrote: Hi Sascha, Thanks for your thorough comments. I have already received comments from Arnd before yours arrived. So some of the commends you two provided are common. I acknowledge most of your opinions, except for two, I have some explanations. +*/ + reg = __raw_readl(MXC_CCM_CACRR); + reg = ~MXC_CCM_CACRR_ARM_PODF_MASK; + reg |= cpu_wp_tbl[wp].cpu_podf MXC_CCM_CACRR_ARM_PODF_OFFSET; + __raw_writel(reg, MXC_CCM_CACRR); We have a simple divider here. Why do we need a reference to struct cpu_wp then? This code could look much simpler. (side note: I'm aware that the original Freescale code is also able to change the cpu frequency using the pll instead of the divider, but is this really necessary?) Using wp_tbl is because that it also contains information like regulator voltage. The clock code does not handle the regulators, not even in the fsl kernel. Since the regulator driver for imx51 have not been upstreamed, you don't see any regulator operation here. Also, like you mentioned, there are two ways to change cpufreq, by post divider or pll change. And post divider change is a simpler way and also the only way for babbage, since cpu freq and ddr freq are all root from the same pll on babbage and they will interfere each other. I understand what you have to do, but the way you did really looks strange. You introduce get_cpu_wp() to get the complete array of workpoints. In the cpufreq driver you have this complete array, pick the desired workpoint, pass the frequency to the clock layer which in turn calls get_cpu_wp() to get the array and loop around it to get the workpoint from the frequency again. Addionally you maintain a pointer to what the clock code thinks the current workpoint is. How about making the clock code agnostic of such workpoints and calculate the pll values and dividers for a given frequency based on the frequency. If that's to complicated you could maintain a table in the clock code. If that's not flexible enough you could pass a pointer to this table to mx51_clocks_init. This array could carry information relevant only to the clock code and only relevant to the i.MX51, In the fsl kernel struct cpu_wp is a catch-all struct for all i.MXs. The i.MX51 uses the pdf/mfi/mfd/mfn/cpu_podf fields, the i.MX35 uses the pll_reg and pdr0_reg fields and who knows what the next i.MX needs. +static struct cpufreq_frequency_table imx_freq_table[4]; Three frequencies should be enough for everyone, right? This should be dynamically allocated like in other cpufreq drivers. Yes, we only support 3 work points, which is for sure. Actually, we only support 2 points on most mx51 chips. I supposed it is ok to use static array here. Please just add dynamic allocation, then we are safe for any potential future use. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
