AARCH64 rootfs built with -mgeneral-regs-only flag?
Hi, We have a AARCH64 verification platform (FPGA) which is built without NEON unit. Is there any prebuilt AARCH64 rootfs (busybox, debian ...) that can be used on such platform? Or is it possible to build an AARCH64 rootfs with -mgeneral-regs-only flag to avoid using NEON unit? Thanks. Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [U-Boot] aarch64-linux-gnu-objdump gives all zeros in init_sequence_f[]
Hi Albert, On Thu, Nov 12, 2015 at 08:20:18AM +0100, Albert ARIBAUD wrote: > Can you provide the target name and commit ID that you are building, > s well as the version of the toolchain that you are building with? > Without being able to reproduce your issue, it's kind of hard to > diagnose it. With the explanation from Ard, I understand the thing now. But thanks for the reply anyway. Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-dev
aarch64-linux-gnu-objdump gives all zeros in init_sequence_f[]
Hi, I need some help to understand aarch64-linux-gnu-objdump output in .data section as below. It's part of the dump of u-boot image with command 'aarch64-linux-gnu-objdump -D -z u-boot'. Disassembly of section .data: 35039898 : 35039948 : 35039948: .word 0x 3503994c: .word 0x 35039950: .word 0x 35039954: .word 0x 35039958: .word 0x 3503995c: .word 0x 35039960: .word 0x 35039964: .word 0x 35039968: .word 0x 3503996c: .word 0x 35039970: .word 0x 35039974: .word 0x 35039978: .word 0x 3503997c: .word 0x 35039980: .word 0x 35039984: .word 0x 35039988: .word 0x 3503998c: .word 0x 35039990: .word 0x 35039994: .word 0x 35039998: .word 0x 3503999c: .word 0x 350399a0: .word 0x 350399a4: .word 0x 350399a8: .word 0x 350399ac: .word 0x 350399b0: .word 0x 350399b4: .word 0x 350399b8: .word 0x 350399bc: .word 0x 350399c0: .word 0x 350399c4: .word 0x 350399c8: .word 0x 350399cc: .word 0x 350399d0: .word 0x 350399d4: .word 0x 350399d8: .word 0x 350399dc: .word 0x 350399e0: .word 0x 350399e4: .word 0x 350399e8: .word 0x 350399ec: .word 0x 350399f0: .word 0x 350399f4: .word 0x 350399f8: .word 0x 350399fc: .word 0x 35039a00: .word 0x 35039a04: .word 0x 35039a08: .word 0x 35039a0c: .word 0x 35039a10: .word 0x 35039a14: .word 0x 35039a18: .word 0x 35039a1c: .word 0x 35039a20: .word 0x 35039a24: .word 0x 35039a28: .word 0x 35039a2c: .word 0x 35039a30: .word 0x 35039a34: .word 0x 35039a38: .word 0x 35039a3c: .word 0x 35039a40: .word 0x 35039a44: .word 0x 35039a48: .word 0x 35039a4c: .word 0x The init_sequence_f[] is an array defined in U-Boot v2015.04 source common/board_f.c, which holds a bunch of pointers to critical initialization functions that have to be called during boot. Obviously, the array cannot be all zeros like what objdump tells. And I confirmed that by printing the pointers at run-time as below. init_sequence_f[0]: 35004280 init_sequence_f[1]: 350042a8 init_sequence_f[2]: 35004380 init_sequence_f[3]: 350042a0 init_sequence_f[4]: 350044b8 init_sequence_f[5]: 35004388 init_sequence_f[6]: 35029538 init_sequence_f[7]: 3500675c init_sequence_f[8]: 3500447c init_sequence_f[9]: 3501d864 init_sequence_f[10]: 35013778 init_sequence_f[11]: 35004398 init_sequence_f[12]: 35028ab8 init_sequence_f[13]: 35004278 init_sequence_f[14]: 35004270 init_sequence_f[15]: 3500445c init_sequence_f[16]: 35001f20 init_sequence_f[17]: 35004574 init_sequence_f[18]: 350042b0 init_sequence_f[19]: 350042c4 init_sequence_f[20]: 350042cc init_sequence_f[21]: 350042f8 init_sequence_f[22]: 350044d4 init_sequence_f[23]: 3500430c init_sequence_f[24]: 35004314 init_sequence_f[25]: 35004330 init_sequence_f[26]: 35004390
Re: git improvements: testers wanted
Hi Andy, On Wed, Apr 22, 2015 at 11:53:37AM -0500, Andy Doan wrote: The Systems team has been working on making clone/pull operations work better for people in different geographical regions: https://wiki.linaro.org/Platform/Systems/GitHA We currently have a DNS test alias named git-geo.linaro.org that should direct users to the server that will deliver the best connection. Before switching over the real git.linaro.org DNS entry, I was hoping to get some more people using this new system to verify its working correctly. = How you can help 1) update git.linaro.org entries in your .git/config files to be git-geo.linaro.org 2) let me know how it goes for you I accessed it from China, and saw basically double speed with git clone using git-geo.linaro.org. Thanks for the improvement \o/ Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Build LSK 3.14 kernel with android-toolchain
+ LAKML and more people. On Mon, Dec 01, 2014 at 05:38:38PM +0100, Arnd Bergmann wrote: On Monday 01 December 2014 16:32:21 Shawn Guo wrote: Is it a valid or supported use case to build LSK 3.14 kernel with android-toolchain? I can build a LSK 3.14 kernel with Linux toolchain gcc-linaro-arm-none-eabi-4.9-2014.09, which boots fine on my board. When I build the same kernel with android-toolchain-eabi-4.9-2014.09-x86, the kernel can be built out successfully, but it fails to boot on the board at very early stage with only uncompressing message shown up like below. Uncompressing Linux... done, booting the kernel. And it's not a LSK 3.14 specific problem, I tried to build mainline 3.10, 3.14 and 3.18-rc4 with the android-toolchain, and they all failed to boot. I need some help to understand if it's a valid use case at all, before I try to looking into the problem. I would expect it to work, it's probably a good idea to find out why it doesn't. For all I know 'arm-none-eabi' is actually /not/ supported for building the kernel, since that doesn't use the Linux Linux variant of eabi, while 'arm-*-linux-gnueabi' or 'arm-*-linux-gnueabihf' is the default for Linux these days and 'arm-*-linux-android' should be compatible with that. Okay. Thanks for the info. It seems that I should download gcc-linaro-arm-linux-gnueabihf-4.9-2014.09 for comparison testing then. Actually, in the very first testing I used arm-linux-gnueabi-gcc 4.7.3 shipped with Ubuntu 14.04. What is the exact target triplet you use in those two cases? They are arm-none-eabi and arm-linux-androideabi. And I also replaced the first toolchain with arm-linux-gnueabi one, and got the same result. A few things you could try: - boot it in qemu using the vexpress or virt platform code, see if the symptom is the same. If it is, attach gdb to the qemu gdbstub to look at the contents of the _log_buf. - Maybe debug_ll crashes, try disabling that - Maybe debug_ll is disabled already, try enabling it to see if you get more output. I tracked it a little bit with debug_ll routine printch() and found it dies at the first pr_info() call in arch/arm/kernel/setup.c: pr_info(Booting Linux on physical CPU 0x%x\n, mpidr); And I spent some time to find out that the issue was introduced by commit dad5451a322b (ARM: 7605/1: vmlinux.lds: Move .notes section next to the rodata) since v3.8 release. Reverting the commit helps me to get a booting kernel that is built by arm-linux-androideabi toolchain. But I do not have the knowledge to understand what is happening. Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Build LSK 3.14 kernel with android-toolchain
On Tue, Dec 02, 2014 at 11:24:03AM +0100, Arnd Bergmann wrote:
I tracked it a little bit with debug_ll routine printch() and found it
dies at the first pr_info() call in arch/arm/kernel/setup.c:
pr_info(Booting Linux on physical CPU 0x%x\n, mpidr);
And I spent some time to find out that the issue was introduced by
commit dad5451a322b (ARM: 7605/1: vmlinux.lds: Move .notes section
next to the rodata) since v3.8 release. Reverting the commit helps me
to get a booting kernel that is built by arm-linux-androideabi
toolchain. But I do not have the knowledge to understand what is
happening.
From my experience in last several years
1. the arm-linux-androideabi- toolchain sets some options by default, PIC
for
example, even -mno-android can't disable all the side effects per my test.
Yes, that's definitely possible. Any idea how the android folks build their
kernel?
2. the arm-linux-eandroideabi- toolchain use gold for linker by default.
Maybe
gold can't understand vmlinux.lds correctly?
That would be very easy to test, just set LD=${CROSS_COMPILE}ld.bfd on the
command line and rebuild. In my testing, I've encountered a number of
different
bugs in both ld.bfd and ld.gold that prevent you from building the kernel.
Yes, either reverting commit dad5451a322b (ARM: 7605/1: vmlinux.lds:
Move .notes section next to the rodata) or building kernel with
LD=${CROSS_COMPILE}ld.bfd get me a working kernel.
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Build LSK 3.14 kernel with android-toolchain
On Tue, Dec 02, 2014 at 06:29:52PM +0800, Jisheng Zhang wrote: On Tue, 2 Dec 2014 02:24:03 -0800 Arnd Bergmann a...@arndb.de wrote: Yes, that's definitely possible. Any idea how the android folks build their kernel? copied from https://android.googlesource.com/toolchain/build/+/HEAD/README The Android toolchain supports the following targets: a. arm-linux-androideabi b. arm-eabi (for Android kernel) c. arm-newlib-eabi (for runnng gcc regression tests) d. i[3456]86-*-linux-gnu, x86_64-*-linux-gnu (for x86 targets) So they build android kernel using the arm-eabi- toolchain. Thanks Jisheng for all the information. Yes, I just built my kernel with arm-eabi in android-toolchain-eabi-4.9-2014.09 and it works fine. So we basically conclude that we should build kernel with arm-eabi rather than arm-linux-androideabi. One thing to note - with CONFIG_ARM_UNWIND turned on, even arm-eabi generate the following warning. This is one difference between android arm-eabi and arm-linux-gnueabi we can see immediately. LD vmlinux arm-eabi-ld: warning: unwinding may not work because EXIDX input section 31 of fs/built-in.o is not in EXIDX output section arm-eabi-ld: warning: unwinding may not work because EXIDX input section 12 of crypto/built-in.o is not in EXIDX output section arm-eabi-ld: warning: unwinding may not work because EXIDX input section 27 of block/built-in.o is not in EXIDX output section arm-eabi-ld: warning: unwinding may not work because EXIDX input section 27 of drivers/built-in.o is not in EXIDX output section arm-eabi-ld: warning: unwinding may not work because EXIDX input section 33 of net/built-in.o is not in EXIDX output section Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Build LSK 3.14 kernel with android-toolchain
Hi all, Is it a valid or supported use case to build LSK 3.14 kernel with android-toolchain? I can build a LSK 3.14 kernel with Linux toolchain gcc-linaro-arm-none-eabi-4.9-2014.09, which boots fine on my board. When I build the same kernel with android-toolchain-eabi-4.9-2014.09-x86, the kernel can be built out successfully, but it fails to boot on the board at very early stage with only uncompressing message shown up like below. Uncompressing Linux... done, booting the kernel. And it's not a LSK 3.14 specific problem, I tried to build mainline 3.10, 3.14 and 3.18-rc4 with the android-toolchain, and they all failed to boot. I need some help to understand if it's a valid use case at all, before I try to looking into the problem. Thanks, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()
On 10 January 2013 16:05, Viresh Kumar viresh.ku...@linaro.org wrote: Another thing, can i have a tested-by from you for both my patches ? remove and add dev? For both: Tested-by: Shawn Guo shawn@linaro.org ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()
On Wed, Jan 09, 2013 at 04:50:44PM +0530, Viresh Kumar wrote: @Shawn: I believe your driver don't require that ugly code anymore (Though i know there is a situation for that to happen, if we have two cpus, you remove second one and then add it back. With this cpufreq_add_dev() would call init() first and then try to match if there are any managed_policies present. But the issue you pointed out about unregistering the driver would be solved by this patch.) Yes, just played it and it works for me. However, I would have to keep that little ugly code in my patch to save the dependency on your patch. Will send a follow-up to clean that up once your patch hits mainline. Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/2] mfd: Add Freescale's PMIC MC34708 support
On 4 July 2012 15:37, Uwe Kleine-König u.kleine-koe...@pengutronix.de wrote: I want to push that forward. What is the state of these patches on your end? Did you start to address the comments? Are there more recent patches than the ones in this thread? Whatever you might have it would be great if you could share it to help preventing duplicate efforts. As far as I know, Paul hasn't been on that effort any more. Copied his another email address. Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: What is right git tree/ branch for imx28/evk device tree work?
Hi Subodh, On 11 May 2012 18:03, Subodh Nijsure snijs...@grid-net.com wrote: Sounds like a very basic question, I would like to test some of the recent patches related to mx28 for freescale EVK board. ( Some thing like - https://lkml.org/lkml/2012/3/13/176 ) Is there specific branch one should be looking for in git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git? Or these changes are being staged some place other than arm-soc.git? I plan to send my mxs/dt/for-3.5 branch to arm-soc soon. Before that happens, you can play the branch below, which has various dependencies solved there. git://git.linaro.org/people/shawnguo/linux-2.6.git mxs/dt/test Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 0/5] more clk-next fixes
On Sun, May 06, 2012 at 10:08:25PM -0700, Mike Turquette wrote: If no one complains about these then I'll commit them to clk-next and (finally) send my pull request to Arnd. On mach-mxs: Tested-by: Shawn Guo shawn@linaro.org Mike, I haven't seen too many outstanding comments on these patches, except the following one that Saravana put on clk-fixed-factor.c, which should be easy to fix. + return (parent_rate / fix-div) * fix-mult; Wouldn't multiplying and then dividing result in better accuracy? Can you please quickly fix it and publish the patches on your clk-next, so that I can ask Arnd to pull my platform porting, on which mach-mxs device tree series depends. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver
On Wed, May 02, 2012 at 08:50:20AM -0500, Rob Lee wrote: --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -21,6 +21,9 @@ #include linux/of_platform.h #include linux/phy.h #include linux/micrel_phy.h +#include linux/export.h +#include linux/cpuidle.h +#include asm/cpuidle.h #include asm/smp_twd.h #include asm/hardware/cache-l2x0.h #include asm/hardware/gic.h @@ -29,6 +32,7 @@ #include asm/system_misc.h #include mach/common.h #include mach/hardware.h +#include mach/cpuidle.h The headers here are mostly sorted in names, so please ... Do you mean sorted alphabetically per group? So the last three includes should look like this instead? ... #include mach/common.h #include mach/cpuidle.h #include mach/hardware.h ... Yes. And do not forget linux/export.h and linux/cpuidle.h. -- Regards, Shawn ___ 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 2 May 2012 21:59, Rob Lee rob@linaro.org wrote:
+ ret = cpuidle_register_device(dev);
+ if (ret) {
+ pr_err(%s: Failed to register cpu %u\n,
+ __func__, cpu_id);
Nit: print ret (error code) too?
I added the printing of the error code based on Sascha's suggestion in
v1 of this submission.
But you did not print ret in above pr_err.
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 0/3] Add imx cpuidle
On Tue, May 01, 2012 at 09:12:37PM -0500, Robert Lee wrote: Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q platform cpuidle implementation. Based on v3.4-rc5 plus recently submitted device tree late_initcall patch: Just to clarify, this is not a device tree late_initcall but a machine specific late_initcall time hook. It has nothing to do with device tree. Regards, Shawn http://www.spinics.net/lists/arm-kernel/msg171620.html Changes since v1: * Removed some unnecessary spaces * Added return value for an error message * Reworked init scheme to use device tree late_initcall. * Moved imx6q and imx5 cpuidle functionality to existing files. Robert Lee (3): ARM: imx: Add common imx cpuidle init functionality. ARM: imx: Add imx5 cpuidle driver ARM: imx: Add imx6q cpuidle driver arch/arm/mach-imx/cpuidle-imx6q.c| 33 arch/arm/mach-imx/mach-imx6q.c | 18 +++ arch/arm/mach-imx/mm-imx5.c | 42 ++-- arch/arm/plat-mxc/Makefile |1 + arch/arm/plat-mxc/cpuidle.c | 80 ++ arch/arm/plat-mxc/include/mach/cpuidle.h | 22 6 files changed, 193 insertions(+), 3 deletions(-) create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c create mode 100644 arch/arm/plat-mxc/cpuidle.c create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h -- 1.7.10 ___ 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
---
arch/arm/plat-mxc/Makefile |1 +
arch/arm/plat-mxc/cpuidle.c | 80
++
arch/arm/plat-mxc/include/mach/cpuidle.h | 22
3 files changed, 103 insertions(+)
create mode 100644 arch/arm/plat-mxc/cpuidle.c
create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index e81290c..63b064b 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -16,6 +16,7 @@ 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..b7a5e1c
--- /dev/null
+++ b/arch/arm/plat-mxc/cpuidle.c
@@ -0,0 +1,80 @@
+/*
+ * 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;
+
+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);
+}
Does this function need to be exported? I haven't seen it being
used anywhere outside this file. Also, can it be __init?
+
+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) {
+ 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);
Nit: print ret (error code) too?
+ goto uninit;
+ }
+ }
+
+ 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; }
Nit: if it can not be in the same line with function name, we usually
have it be:
{
return -ENODEV;
}
+#endif
--
1.7.10
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver
On Tue, May 01, 2012 at 09:12:40PM -0500, Robert Lee wrote:
Add basic imx6q cpuidle driver. For now, only basic WFI state is
supported. Deeper idle states will be added in the future.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-imx/cpuidle-imx6q.c | 33 +
So, this file is not needed any more, I guess.
arch/arm/mach-imx/mach-imx6q.c| 18 ++
2 files changed, 51 insertions(+)
create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c
b/arch/arm/mach-imx/cpuidle-imx6q.c
new file mode 100644
index 000..b74557f
--- /dev/null
+++ b/arch/arm/mach-imx/cpuidle-imx6q.c
@@ -0,0 +1,33 @@
+/*
+ * 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/init.h
+#include linux/cpuidle.h
+#include linux/export.h
+#include asm/cpuidle.h
+#include mach/cpuidle.h
+
+static struct cpuidle_driver imx6q_cpuidle_driver = {
+ .name = imx6q_cpuidle,
+ .owner = THIS_MODULE,
+ .en_core_tk_irqen = 1,
+ .states[0] = ARM_CPUIDLE_WFI_STATE,
+ .state_count= 1,
+};
+
+int __init imx6q_cpuidle_init(void)
+{
+ imx_cpuidle_set_driver(imx6q_cpuidle_driver);
+
+ return 0;
+}
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index da6c1d9..21e2051 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -21,6 +21,9 @@
#include linux/of_platform.h
#include linux/phy.h
#include linux/micrel_phy.h
+#include linux/export.h
+#include linux/cpuidle.h
+#include asm/cpuidle.h
#include asm/smp_twd.h
#include asm/hardware/cache-l2x0.h
#include asm/hardware/gic.h
@@ -29,6 +32,7 @@
#include asm/system_misc.h
#include mach/common.h
#include mach/hardware.h
+#include mach/cpuidle.h
The headers here are mostly sorted in names, so please ...
Regards,
Shawn
void imx6q_restart(char mode, const char *cmd)
{
@@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void)
imx6q_pm_init();
}
+static struct cpuidle_driver imx6q_cpuidle_driver = {
+ .name = imx6q_cpuidle,
+ .owner = THIS_MODULE,
+ .en_core_tk_irqen = 1,
+ .states[0] = ARM_CPUIDLE_WFI_STATE,
+ .state_count= 1,
+};
+
+static void __init imx6q_init_late(void)
+{
+ imx_cpuidle_init(imx6q_cpuidle_driver);
+}
+
static void __init imx6q_map_io(void)
{
imx_lluart_map_io();
@@ -142,6 +159,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
--
1.7.10
___
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 24, 2012 at 08:54:26AM +0100, Russell King - ARM Linux wrote: On Tue, Apr 24, 2012 at 09:38:43AM +0800, Shawn Guo wrote: On Mon, Apr 23, 2012 at 10:45:02AM -0500, Rob Lee wrote: Let me try last time. What about having a late_initcall hook in machine_desc? Also fine with me. Shall I add Shawn's patch to my imx cpuidle patchset or should the arch/arm/kernel/setup.c and arch.h changes be submitted separately? If separately, Shawn, did you want to submit this patch or should I? Strange. Russell is not in the Cc list. I remember I added Russell into Cc when I propose the idea. Added him again. I didn't see any message in this thread cc'd to me, but that's not to say I hadn't already read this patch. I don't have any comment against it, but I do wonder how often this hook would be used. I guess mach-* that use common cpuidle will likely need this hook. We do seem to have quite a number of late_initcall()s in arch/arm/mach-*, so it seems to be a good idea - provided someone's willing to convert all those users of late_initcall()s. Agreed. The late_initcall()s in arch/arm/mach-* will not scale for long time, since we are moving toward single build. -- Regards, Shawn ___ 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 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.
--
Regards,
Shawn
___
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 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:
...
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.
Let me try last time. What about having a late_initcall hook in
machine_desc?
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
___
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 10:45:02AM -0500, Rob Lee wrote:
Let me try last time. What about having a late_initcall hook in
machine_desc?
Also fine with me.
Shall I add Shawn's patch to my imx cpuidle patchset or should the
arch/arm/kernel/setup.c and arch.h changes be submitted separately?
If separately, Shawn, did you want to submit this patch or should I?
Strange. Russell is not in the Cc list. I remember I added Russell
into Cc when I propose the idea. Added him again.
Rob,
I suggest you have changes on arch/arm/kernel/setup.c and arch.h be
a separate patch, but you can still have it in the series to show why
we need the changes. Cc Russell when posting the series, and see if
Russell is fine with the patch. If he is, we can ask his preference
how the patch should go in, submitting it to his patch tracker or we
can have it go though arm-soc along with the series to save the
dependency.
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
___
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 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
*/
}
--
Regards,
Shawn
___
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 Thu, Apr 19, 2012 at 08:43:08AM +0200, Sascha Hauer wrote: Sascha or Shawn, any further comments on my question? Sorry for the late response, Rob. 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. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 13/13] clk: basic: improve parent_names return errors
On 17 April 2012 11:50, Turquette, Mike mturque...@ti.com wrote: That is a good question. I think it is worth waiting on Saravana's patch which exposes non-private members of struct clk via struct clk_hw. This will have an effect on both platform clock data and code. Saravana, (*nudge*) (*nudge*) goes to you now :) Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 13/13] clk: basic: improve parent_names return errors
On 17 April 2012 07:10, Turquette, Mike mturque...@ti.com wrote: ... Yes, this was a braindead change on my part. I'll remove the kstrdup in my next series (the rest of this patch will stay in). Do you have an ETA on that? A few platform porting are waiting for a stable branch with all necessary fixup/cleanup integrated to publish the patches. Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3] mfd: da9052: add device-tree support for i2c driver
On Sat, Apr 14, 2012 at 09:39:06PM +0800, Ying-Chun Liu (PaulLiu) wrote:
From: Ying-Chun Liu (PaulLiu) paul@linaro.org
This patch adds device-tree support for dialog MFD and the binding
documentations.
Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org
Cc: Samuel Ortiz sa...@linux.intel.com
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Shawn Guo shawn@linaro.org
Cc: Ashish Jangam ashish.jan...@kpitcummins.com
---
.../devicetree/bindings/mfd/da9052-i2c.txt | 60
drivers/mfd/da9052-i2c.c | 51 ++---
2 files changed, 103 insertions(+), 8 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/da9052-i2c.txt
diff --git a/Documentation/devicetree/bindings/mfd/da9052-i2c.txt
b/Documentation/devicetree/bindings/mfd/da9052-i2c.txt
new file mode 100644
index 000..1857f4a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/da9052-i2c.txt
@@ -0,0 +1,60 @@
+* Dialog DA9052/53 Power Management Integrated Circuit (PMIC)
+
+Required properties:
+- compatible : Should be dlg,da9052, dlg,da9053-aa,
+ dlg,da9053-ab, or dlg,da9053-bb
+
+Sub-nodes:
+- regulators : Contain the regulator nodes. The DA9052/53 regulators are
+ bound using their names as listed below:
+
+buck0 : regulator BUCK0
+buck1 : regulator BUCK1
+buck2 : regulator BUCK2
+buck3 : regulator BUCK3
+ldo4 : regulator LDO4
+ldo5 : regulator LDO5
+ldo6 : regulator LDO6
+ldo7 : regulator LDO7
+ldo8 : regulator LDO8
+ldo9 : regulator LDO9
+ldo10 : regulator LDO10
+ldo11 : regulator LDO11
+ldo12 : regulator LDO12
+ldo13 : regulator LDO13
+
+ The bindings details of individual regulator device can be found in:
+ Documentation/devicetree/bindings/regulator/regulator.txt
+
+Examples:
+
+i2c@63fc8000 { /* I2C1 */
+ status = okay;
+
+ pmic: dialog@48 {
+ compatible = dlg,da9053-aa;
+ reg = 0x48;
+
+ regulators {
+ buck0 {
+ regulator-min-microvolt = 50;
+ regulator-max-microvolt = 2075000;
+ };
+
+ buck1 {
+ regulator-min-microvolt = 50;
+ regulator-max-microvolt = 2075000;
+ };
+
+ buck2 {
+ regulator-min-microvolt = 925000;
+ regulator-max-microvolt = 250;
+ };
+
+ buck3 {
+ regulator-min-microvolt = 925000;
+ regulator-max-microvolt = 250;
+ };
+ };
+ };
+};
diff --git a/drivers/mfd/da9052-i2c.c b/drivers/mfd/da9052-i2c.c
index 36b88e3..b946b0ff 100644
--- a/drivers/mfd/da9052-i2c.c
+++ b/drivers/mfd/da9052-i2c.c
@@ -22,6 +22,11 @@
#include linux/mfd/da9052/da9052.h
#include linux/mfd/da9052/reg.h
+#ifdef CONFIG_OF
+#include linux/of.h
+#include linux/of_device.h
+#endif
+
I'm not a big fan of #ifdef CONFIG_OF, but maintainers may like
to compile out DT support for non-DT build, so I leave it there.
static int da9052_i2c_enable_multiwrite(struct da9052 *da9052)
{
int reg_val, ret;
@@ -41,6 +46,24 @@ static int da9052_i2c_enable_multiwrite(struct da9052
*da9052)
return 0;
}
+static struct i2c_device_id da9052_i2c_id[] = {
+ {da9052, DA9052},
+ {da9053-aa, DA9053_AA},
+ {da9053-ba, DA9053_BA},
+ {da9053-bb, DA9053_BB},
+ {}
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id dialog_dt_ids[] = {
+ { .compatible = dlg,da9052, .data = da9052_i2c_id[0] },
+ { .compatible = dlg,da9053-aa, .data = da9052_i2c_id[1] },
+ { .compatible = dlg,da9053-ab, .data = da9052_i2c_id[2] },
+ { .compatible = dlg,da9053-bb, .data = da9052_i2c_id[3] },
+ { /* sentinel */ }
+};
+#endif
+
static int __devinit da9052_i2c_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -76,6 +99,23 @@ static int __devinit da9052_i2c_probe(struct i2c_client
*client,
if (ret 0)
goto err_regmap;
+#ifdef CONFIG_OF
+ if (!id) {
+ int i;
What is this for?
+ struct device_node *np = client-dev.of_node;
+ const struct of_device_id *deviceid;
+
+ deviceid = of_match_node(np, dialog_dt_ids);
+ id = (const struct i2c_device_id *)(deviceid-data);
Unnecessary parentheses around deviceid-data.
Regards,
Shawn
+ }
+#endif
+
+ if (!id) {
+ ret = -ENODEV;
+ dev_err(client-dev, id is null.\n);
+ goto err_regmap
Re: [PATCH 13/13] clk: basic: improve parent_names return errors
On Wed, Apr 11, 2012 at 06:02:51PM -0700, Mike Turquette wrote:
...
@@ -175,23 +188,32 @@ struct clk *clk_register_divider(struct device *dev,
const char *name,
div-flags = clk_divider_flags;
div-lock = lock;
+ /* allocate the temporary parent_names */
if (parent_name) {
- div-parent[0] = kstrdup(parent_name, GFP_KERNEL);
- if (!div-parent[0])
- goto out;
+ parent_names[0] = kstrdup(parent_name, GFP_KERNEL);
+ if (!parent_names[0]) {
+ pr_err(%s: could not allocate parent_names\n,
+ __func__);
+ goto fail_parent_names;
+ }
}
Why do we need to copy the parent_names here at all? clk_register()
has done that for each basic clk.
Regards,
Shawn
+ /* register the clock */
clk = clk_register(dev, name,
clk_divider_ops, div-hw,
- div-parent,
+ (parent_name ? parent_names: NULL),
(parent_name ? 1 : 0),
flags);
- if (clk)
- return clk;
-out:
- kfree(div-parent[0]);
- kfree(div);
+ /* free the temporary parent_names */
+ if (parent_name)
+ kfree(parent_names[0]);
+
+ if (!IS_ERR(clk))
+ goto out;
- return NULL;
+fail_parent_names:
+ kfree(div);
+out:
+ return clk;
}
___
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 Thu, Apr 12, 2012 at 11:14:38AM +, Arnd Bergmann wrote: On Thursday 12 April 2012, 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 think pulling it in through the arm-soc tree is still ok, but it's borderline because of the size and patch 13 is probably too big, in addition to the comments that were made there. Let's pull patches 1 through 12 in to a separate series that we don't mix with the other bug fixes. Mike, please send a pull request with the Acks added in. I just appended 3 more patches to the series. Patches #1 and #2 change the interface between clk core and clk drivers - clk_ops a little bit, (this is something Mike acked a couple of weeks ago, but missed from the series) and patch #3 is a critical bug fix. So unless we can send the whole series for -rc, I'd vote we send none of them for -rc. Instead, we can stabilize it somewhere and ask all the clk driver porting base on that. Sending part of the cleanup/fixup and leaving the other that could require changes on clk drivers out is a bad idea to me. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2] ARM: dts: imx6q: add anatop regulators
On Fri, Mar 30, 2012 at 09:46:53PM +0800, Ying-Chun Liu (PaulLiu) wrote:
From: Ying-Chun Liu (PaulLiu) paul@linaro.org
Add anatop regulators to imx6q.dtsi for all imx6q platforms.
Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org
Signed-off-by: Richard Zhao richard.z...@linaro.org
Cc: Shawn Guo shawn@linaro.org
---
arch/arm/boot/dts/imx6q.dtsi | 86
++
1 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 263e8f3..79f59e7 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -346,6 +346,92 @@
compatible = fsl,imx6q-anatop;
reg = 0x020c8000 0x1000;
interrupts = 0 49 0x04 0 54 0x04 0 127 0x04;
+ #address-cells = 1;
+ #size-cells = 0;
These two lines are not needed now. Removed them and applied the
patch. Thanks.
Regards,
Shawn
+
+ regulator-1p1@110 {
+ compatible = fsl,anatop-regulator;
+ regulator-name = vdd1p1;
+ regulator-min-microvolt = 80;
+ regulator-max-microvolt = 1375000;
+ regulator-always-on;
+ anatop-reg-offset = 0x110;
+ anatop-vol-bit-shift = 8;
+ anatop-vol-bit-width = 5;
+ anatop-min-bit-val = 4;
+ anatop-min-voltage = 80;
+ anatop-max-voltage = 1375000;
+ };
+
+ regulator-3p0@120 {
+ compatible = fsl,anatop-regulator;
+ regulator-name = vdd3p0;
+ regulator-min-microvolt = 280;
+ regulator-max-microvolt = 315;
+ regulator-always-on;
+ anatop-reg-offset = 0x120;
+ anatop-vol-bit-shift = 8;
+ anatop-vol-bit-width = 5;
+ anatop-min-bit-val = 0;
+ anatop-min-voltage = 2625000;
+ anatop-max-voltage = 340;
+ };
+
+ regulator-2p5@130 {
+ compatible = fsl,anatop-regulator;
+ regulator-name = vdd2p5;
+ regulator-min-microvolt = 200;
+ regulator-max-microvolt = 275;
+ regulator-always-on;
+ anatop-reg-offset = 0x130;
+ anatop-vol-bit-shift = 8;
+ anatop-vol-bit-width = 5;
+ anatop-min-bit-val = 0;
+ anatop-min-voltage = 200;
+ anatop-max-voltage = 275;
+ };
+
+ regulator-vddcore@140 {
+ compatible = fsl,anatop-regulator;
+ regulator-name = cpu;
+ regulator-min-microvolt = 725000;
+ regulator-max-microvolt = 145;
+ regulator-always-on;
+ anatop-reg-offset = 0x140;
+ anatop-vol-bit-shift = 0;
+ anatop-vol-bit-width = 5;
+ anatop-min-bit-val = 1;
+ anatop-min-voltage = 725000;
+ anatop-max-voltage = 145;
+ };
+
+ regulator-vddpu@140 {
+ compatible = fsl,anatop-regulator;
+ regulator-name = vddpu;
+ regulator-min-microvolt = 725000;
+ regulator-max-microvolt = 145;
+ regulator-always-on;
+ anatop-reg-offset = 0x140;
+ anatop-vol-bit-shift = 9;
+ anatop-vol-bit-width = 5
Re: [PATCH] Regulator: anatop-regulator: patching to device-tree property reg.
On 27 March 2012 15:54, Ying-Chun Liu (PaulLiu) paul@linaro.org wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org Change reg to anatop-reg-offset due to there is a warning of handling no size field in reg. This patch also adds the missing device-tree binding documentation. Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org Cc: Shawn Guo shawn@linaro.org Cc: Mark Brown broo...@opensource.wolfsonmicro.com Cc: Liam Girdwood l...@ti.com Acked-by: Shawn Guo shawn@linaro.org ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v13] Regulator: Add Anatop regulator driver
On Thu, Mar 22, 2012 at 02:49:36PM +0800, Ying-Chun Liu (PaulLiu) wrote:
...
Ouch. I'll prepare a separate patch to add back the documentation.
I just gave a quick testing on the driver with the dts change you
posted on imx6. There is some little problem we may need to address.
prom_parse: Bad cell count for
/soc/aips-bus@0200/anatop@020c8000/regulator-vddpu@140
vddpu: 725 -- 1300 mV at 1100 mV
prom_parse: Bad cell count for
/soc/aips-bus@0200/anatop@020c8000/regulator-vddcore@140
cpu: 725 -- 1300 mV at 1100 mV
prom_parse: Bad cell count for
/soc/aips-bus@0200/anatop@020c8000/regulator-vddsoc@140
vddsoc: 725 -- 1300 mV at 1100 mV
prom_parse: Bad cell count for
/soc/aips-bus@0200/anatop@020c8000/regulator-2p5@130
vdd2p5: 2000 -- 2775 mV at 2400 mV
prom_parse: Bad cell count for
/soc/aips-bus@0200/anatop@020c8000/regulator-1p1@110
vdd1p1: 800 -- 1400 mV at 1100 mV
prom_parse: Bad cell count for
/soc/aips-bus@0200/anatop@020c8000/regulator-3p0@120
vdd3p0: 2800 -- 3150 mV at 3000 mV
The DT core function __of_translate_address will give some annoying
error messages. We call of_platform_populate from anatop mfd driver
to populate regulator device, and during the call, DT core tries to
translate reg property to address resource, and will complain if
!(#size-cells 0).
To fix that, we may want to rename reg property to something else,
e.g. anatop-reg-offset. I tested the following changes removed the
error messages above.
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 56d3c16..65bad45 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -346,16 +346,14 @@
compatible = fsl,imx6q-anatop;
reg = 0x020c8000 0x1000;
interrupts = 0 49 0x04 0 54 0x04 0 127 0x04;
- #address-cells = 1;
- #size-cells = 0;
- regulator-vddpu@140 {
+ regulator-vddpu {
compatible = fsl,anatop-regulator;
regulator-name = vddpu;
regulator-min-microvolt = 725000;
regulator-max-microvolt = 130;
regulator-always-on;
- reg = 0x140;
+ anatop-reg-offset = 0x140;
anatop-vol-bit-shift = 9;
anatop-vol-bit-width = 5;
anatop-min-bit-val = 1;
@@ -363,13 +361,13 @@
anatop-max-voltage = 130;
};
- regulator-vddcore@140 {
+ regulator-vddcore {
compatible = fsl,anatop-regulator;
regulator-name = cpu;
regulator-min-microvolt = 725000;
regulator-max-microvolt = 130;
regulator-always-on;
- reg = 0x140;
+ anatop-reg-offset = 0x140;
anatop-vol-bit-shift = 0;
anatop-vol-bit-width = 5;
anatop-min-bit-val = 1;
@@ -377,13 +375,13 @@
anatop-max-voltage = 130;
};
- regulator-vddsoc@140 {
+ regulator-vddsoc {
compatible = fsl,anatop-regulator;
regulator-name = vddsoc;
regulator-min-microvolt = 725000;
regulator-max-microvolt = 130;
regulator-always-on;
- reg = 0x140;
+ anatop-reg-offset = 0x140;
anatop-vol-bit-shift = 18;
anatop-vol-bit-width = 5;
anatop-min-bit-val = 1;
@@ -391,13 +389,13 @@
anatop-max-voltage = 130;
};
- regulator-2p5@130 {
+ regulator-2p5 {
compatible = fsl,anatop-regulator;
regulator-name = vdd2p5;
regulator-min-microvolt = 200;
Re: [PATCH] ARM: dts: imx6q: add anatop regulators
On Tue, Mar 20, 2012 at 11:28:59AM +0800, Ying-Chun Liu (PaulLiu) wrote:
From: Ying-Chun Liu (PaulLiu) paul@linaro.org
Add anatop regulators to imx6q.dtsi for all imx6q platforms.
I would expect all those regulator device nodes be sorted by
register offset, and for the devices sharing the same register they
can be sorted by anatop-vol-bit-shift.
I'm looking at the IMX6DQRM Rev. C, and commenting the differences
I'm seeing from the document below.
Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org
Signed-off-by: Richard Zhao richard.z...@linaro.org
Cc: Shawn Guo shawn@linaro.org
---
arch/arm/boot/dts/imx6q.dtsi | 86
++
1 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 263e8f3..dd41514 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -346,6 +346,92 @@
compatible = fsl,imx6q-anatop;
reg = 0x020c8000 0x1000;
interrupts = 0 49 0x04 0 54 0x04 0 127 0x04;
+ #address-cells = 1;
+ #size-cells = 0;
+
+ regulator-vddpu@140 {
+ compatible = fsl,anatop-regulator;
+ regulator-name = vddpu;
+ regulator-min-microvolt = 725000;
+ regulator-max-microvolt = 130;
+ regulator-always-on;
+ reg = 0x140;
+ anatop-vol-bit-shift = 9;
+ anatop-vol-bit-width = 5;
+ anatop-min-bit-val = 1;
+ anatop-min-voltage = 725000;
+ anatop-max-voltage = 130;
1.450V
+ };
+
+ regulator-vddcore@140 {
+ compatible = fsl,anatop-regulator;
+ regulator-name = cpu;
+ regulator-min-microvolt = 725000;
+ regulator-max-microvolt = 130;
+ regulator-always-on;
+ reg = 0x140;
+ anatop-vol-bit-shift = 0;
+ anatop-vol-bit-width = 5;
+ anatop-min-bit-val = 1;
+ anatop-min-voltage = 725000;
+ anatop-max-voltage = 130;
Ditto
+ };
+
+ regulator-vddsoc@140 {
+ compatible = fsl,anatop-regulator;
+ regulator-name = vddsoc;
+ regulator-min-microvolt = 725000;
+ regulator-max-microvolt = 130;
+ regulator-always-on;
+ reg = 0x140;
+ anatop-vol-bit-shift = 18;
+ anatop-vol-bit-width = 5;
+ anatop-min-bit-val = 1;
+ anatop-min-voltage = 725000;
+ anatop-max-voltage = 130;
Ditto
+ };
+
+ regulator-2p5@130 {
+ compatible = fsl,anatop-regulator;
+ regulator-name = vdd2p5;
+ regulator-min-microvolt = 200;
+ regulator-max-microvolt = 2775000;
+ regulator-always-on;
+ reg = 0x130;
+ anatop-vol-bit-shift = 8;
+ anatop-vol-bit-width = 5;
+ anatop-min-bit-val = 0;
+ anatop-min-voltage = 200;
+ anatop-max-voltage = 2775000;
2.75V
+ };
+
+ regulator-1p1@110 {
+ compatible = fsl,anatop-regulator;
+ regulator-name = vdd1p1;
+ regulator-min-microvolt = 80;
+ regulator-max-microvolt = 140;
+ regulator-always-on;
+ reg = 0x110
Re: [PATCH] rtc: add support for Freescale SNVS RTC
On Mon, Mar 19, 2012 at 09:04:29PM +0800, Ying-Chun Liu (PaulLiu) wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org This adds an RTC driver for the Low Power (LP) section of SNVS. It hooks into the /dev/rtc interface and supports ioctl to complete RTC features. This driver supports device tree bindings. Then, you need to have a document in Documentation/devicetree/bindings. It only uses the RTC hw in non-secure mode. Signed-off-by: Anish Trivedi an...@freescale.com Signed-off-by: Eric Miao eric.m...@linaro.org Signed-off-by: Anson Huang b20...@freescale.com Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org Cc: Alessandro Zummo a.zu...@towertech.it Cc: Shawn Guo shawn@linaro.org --- drivers/rtc/Kconfig| 11 + drivers/rtc/Makefile |1 + drivers/rtc/rtc-snvs.c | 737 3 files changed, 749 insertions(+), 0 deletions(-) create mode 100644 drivers/rtc/rtc-snvs.c diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index 3a125b8..d58f4b7 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -634,6 +634,17 @@ config RTC_MXC This driver can also be built as a module, if so, the module will be called rtc-mxc. +config RTC_DRV_SNVS + tristate Freescale SNVS Real Time Clock + depends on ARCH_MXC + depends on RTC_CLASS I'm not sure this is really necessary, since this config is included in if RTC_CLASS. + help +If you say yes here you get support for the Freescale SNVS +Low Power (LP) RTC module. + +This driver can also be built as a module, if so, the module +will be called rtc-snvs. + config RTC_DRV_BQ4802 tristate TI BQ4802 help diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile index 6e69823..8b30686 100644 --- a/drivers/rtc/Makefile +++ b/drivers/rtc/Makefile @@ -93,6 +93,7 @@ obj-$(CONFIG_RTC_DRV_S35390A) += rtc-s35390a.o obj-$(CONFIG_RTC_DRV_S3C)+= rtc-s3c.o obj-$(CONFIG_RTC_DRV_SA1100) += rtc-sa1100.o obj-$(CONFIG_RTC_DRV_SH) += rtc-sh.o +obj-$(CONFIG_RTC_DRV_SNVS) += rtc-snvs.o obj-$(CONFIG_RTC_DRV_SPEAR) += rtc-spear.o obj-$(CONFIG_RTC_DRV_STARFIRE) += rtc-starfire.o obj-$(CONFIG_RTC_DRV_STK17TA8) += rtc-stk17ta8.o diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c new file mode 100644 index 000..49ac8a5 --- /dev/null +++ b/drivers/rtc/rtc-snvs.c @@ -0,0 +1,737 @@ +/* + * Copyright (C) 2011 Freescale Semiconductor, Inc. + */ + +/* + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ +/* + * Implementation based on rtc-ds1553.c + */ + +/*! I'm not sure /*! is the format for kernel-doc. Please take a look at Documentation/kernel-doc-nano-HOWTO.txt. + * @defgroup RTC Real Time Clock (RTC) Driver + */ +/*! + * @file rtc-snvs.c + * @brief Secure Real Time Clock (SRTC) interface in Freescale's SNVS module + * + * This file contains Real Time Clock interface for Linux. The Freescale + * SNVS module's Low Power (LP) SRTC functionality is utilized in this driver, + * in non-secure mode. + * + * @ingroup RTC + */ + I feel above several sections of documents can just be in one multiple line comment section. +#include linux/slab.h +#include linux/delay.h +#include linux/rtc.h +#include linux/module.h +#include linux/fs.h +#include linux/init.h +#include linux/interrupt.h +#include linux/platform_device.h +#include linux/uaccess.h +#include linux/io.h +#include linux/sched.h +#include linux/of.h +#include linux/of_device.h + +/* Register definitions */ +#define SNVS_HPSR 0x14/* HP Status Register */ Normally, we have one space rather than tab after #define. +#define SNVS_LPCR 0x38/* LP Control Register */ +#define SNVS_LPSR 0x4C/* LP Status Register */ +#define SNVS_LPSRTCMR 0x50/* LP Secure Real Time Counter MSB Register */ +#define SNVS_LPSRTCLR 0x54/* LP Secure Real Time Counter LSB Register */ +#define SNVS_LPTAR 0x58/* LP Time Alarm Register */ +#define SNVS_LPSMCMR0x5C/* LP Secure Monotonic Counter MSB Register */ +#define SNVS_LPSMCLR0x60/* LP Secure Monotonic Counter LSB
Re: [PATCH v13] Regulator: Add Anatop regulator driver
On Wed, Mar 14, 2012 at 10:29:12AM +0800, Ying-Chun Liu (PaulLiu) wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org Anatop is an integrated regulator inside i.MX6 SoC. There are 3 digital regulators which controls PU, CORE (ARM), and SOC. And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB). This patch adds the Anatop regulator driver. Signed-off-by: Nancy Chen nancy.c...@freescale.com Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org Acked-by: Shawn Guo shawn@linaro.org Reviewed-by: Axel Lin axel@gmail.com Cc: Mark Brown broo...@opensource.wolfsonmicro.com Cc: Liam Girdwood l...@ti.com Cc: Samuel Ortiz sa...@linux.intel.com Cc: Jean-Christophe PLAGNIOL-VILLARD plagn...@jcrosoft.com --- drivers/regulator/Kconfig|8 + drivers/regulator/Makefile |1 + drivers/regulator/anatop-regulator.c | 241 ++ 3 files changed, 250 insertions(+), 0 deletions(-) create mode 100644 drivers/regulator/anatop-regulator.c I just noticed that the binding document below got lost since v11 of the series. Documentation/devicetree/bindings/regulator/anatop-regulator.txt It got lost by mistaken or you dropped it for some reason? -- Regards, Shawn ___ 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 Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote:
...
+struct clk_ops {
+ int (*prepare)(struct clk_hw *hw);
+ void(*unprepare)(struct clk_hw *hw);
+ int (*enable)(struct clk_hw *hw);
+ void(*disable)(struct clk_hw *hw);
+ int (*is_enabled)(struct clk_hw *hw);
+ unsigned long (*recalc_rate)(struct clk_hw *hw,
+ unsigned long parent_rate);
I believe I have heard people love the interface with parent_rate
passed in. I love that too. But I would like to ask the same thing
on .round_rate and .set_rate as well for the same reason why we have
it for .recalc_rate.
+ long(*round_rate)(struct clk_hw *hw, unsigned long,
+ unsigned long *);
Yes, we already have parent_rate passed in .round_rate with an pointer.
But I think it'd be better to have it passed in no matter flag
CLK_SET_RATE_PARENT is set or not. Something like:
8---
@@ -584,7 +584,7 @@ EXPORT_SYMBOL_GPL(clk_get_rate);
*/
unsigned long __clk_round_rate(struct clk *clk, unsigned long rate)
{
- unsigned long unused;
+ unsigned long parent_rate = 0;
if (!clk)
return -EINVAL;
@@ -592,10 +592,10 @@ unsigned long __clk_round_rate(struct clk *clk, unsigned
long rate)
if (!clk-ops-round_rate)
return clk-rate;
- if (clk-flags CLK_SET_RATE_PARENT)
- return clk-ops-round_rate(clk-hw, rate, unused);
- else
- return clk-ops-round_rate(clk-hw, rate, NULL);
+ if (clk-parent)
+ parent_rate = clk-parent-rate;
+
+ return clk-ops-round_rate(clk-hw, rate, parent_rate);
}
/**
@@ -765,9 +765,12 @@ static void clk_calc_subtree(struct clk *clk, unsigned
long new_rate)
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;
+ unsigned long best_parent_rate = 0;
unsigned long new_rate;
+ if (clk-parent)
+ best_parent_rate = clk-parent-rate;
+
if (!clk-ops-round_rate !(clk-flags CLK_SET_RATE_PARENT)) {
clk-new_rate = clk-rate;
return NULL;
@@ -780,10 +783,7 @@ static struct clk *clk_calc_new_rates(struct clk *clk,
unsigned long rate)
goto out;
}
- if (clk-flags CLK_SET_RATE_PARENT)
- new_rate = clk-ops-round_rate(clk-hw, rate,
best_parent_rate);
- else
- new_rate = clk-ops-round_rate(clk-hw, rate, NULL);
+ new_rate = clk-ops-round_rate(clk-hw, rate, best_parent_rate);
if (best_parent_rate != clk-parent-rate) {
top = clk_calc_new_rates(clk-parent, best_parent_rate);
---8
The reason behind the change is that any clk implements .round_rate will
mostly need to get the parent rate, no matter flag CLK_SET_RATE_PARENT
is set or not. Instead of expecting every .round_rate implementation
to get the parent rate in the way beloe
parent_rate = __clk_get_rate(__clk_get_parent(hw-clk));
we can just pass the parent rate into .round_rate.
+ int (*set_parent)(struct clk_hw *hw, u8 index);
+ u8 (*get_parent)(struct clk_hw *hw);
+ int (*set_rate)(struct clk_hw *hw, unsigned long);
For the same reason, I would change .set_rate interface like below.
8---
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index d5ac6a7..6bd8037 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -119,7 +119,8 @@ static long clk_divider_round_rate(struct clk_hw *hw,
unsigned long rate,
}
EXPORT_SYMBOL_GPL(clk_divider_round_rate);
-static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate)
+static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
{
struct clk_divider *divider = to_clk_divider(hw);
unsigned int div;
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index dbc310a..d74ac4b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -833,17 +833,18 @@ static struct clk *clk_propagate_rate_change(struct clk
*clk, unsigned long even
static void clk_change_rate(struct clk *clk)
{
struct clk *child;
- unsigned long old_rate;
+ unsigned long old_rate, parent_rate = 0;
struct hlist_node *tmp;
old_rate = clk-rate;
+ if (clk-parent)
+ parent_rate = clk-parent-rate;
if (clk-ops-set_rate)
- clk-ops-set_rate(clk-hw, clk-new_rate);
+ clk-ops-set_rate(clk-hw, clk-new_rate, parent_rate);
if (clk-ops-recalc_rate)
- clk-rate = clk-ops-recalc_rate(clk-hw,
- clk-parent-rate);
+ clk-rate = clk-ops-recalc_rate(clk-hw, parent_rate);
Re: [PATCH v7 2/3] clk: introduce the common clock framework
On 21 March 2012 07:46, Turquette, Mike mturque...@ti.com wrote: ... As mentioned above, you'll still need to check for CLK_SET_RATE_PARENT in your .round_rate implementation with __clk_get_flags(hw-clk). For my particular case, the clk is PLL with fixed rate clk (oscillator) as parent. It's known that flag CLK_SET_RATE_PARENT will never be set for this type of clks. Did you want to send a formal patch or just have me absorb this into the fixes I'm brewing already? I've already fixed the potential null ptr dereferences in clk_calc_new_rates, but that's no big deal. The code was attached for better discussion, and I would leave the formal patch to you. Regards, Shawn ___ 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 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. -- Regards, Shawn ___ 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 Fri, Mar 16, 2012 at 08:23:57PM -0700, Saravana Kannan wrote: On 03/07/2012 01:20 PM, Turquette, Mike wrote: ... Admittedly I think that the OMAP code could migrate some of these bits to a lazy-registration model, specifically the hwmod object instances, but that requires an awful lot of refactoring for a fairly large stack of platform code. This might be something to achieve in the future but for now we *need* initialisation to be fully static. When we work on moving clocks to device tree, wouldn't you face the same problem? You will have to dynamically create most of the clocks in that case too. From what I heard from Mike[1], Omap will not have most of the clocks encoded in device tree. Although my original preference was to have all the clocks represented in device tree and dynamically registered to clk framework, I've heard people including Grant incline to only have oscillator and leaf modules clocks in device tree. I somehow agree with that now, because having all the clocks in there will bloat device tree dramatically, considering there will be 3 clks, clk_gate, clk_divider and clk_mux backing one clock in general. Assuming we would agree to have most SoC internal clocks registered from clock driver rather than device tree, I would like to hear about how we should have these clock registered from clock driver, static initialization or using register function? -- Regards, Shawn [1] http://article.gmane.org/gmane.linux.linaro.devel/10554 ___ 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
Reading the documentation of function clk_set_rate(), I'm not sure it exactly matches what the code does. If there is mismatch, it might be worth sending an incremental patch to update the documentation and avoid the confusion? On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote: +/** + * clk_set_rate - specify a new rate for clk + * @clk: the clk whose rate is being changed + * @rate: the new rate for clk + * + * In the simplest case clk_set_rate will only change the rate of clk. + * + * If clk has the CLK_SET_RATE_GATE flag set and it is enabled this call + * will fail; only when the clk is disabled will it be able to change + * its rate. + * + * Setting the CLK_SET_RATE_PARENT flag allows clk_set_rate to + * recursively propagate up to clk's parent; whether or not this happens + * depends on the outcome of clk's .round_rate implementation. If + * *parent_rate is 0 after calling .round_rate then upstream parent Might *parent_rate is not changed be more accurate? + * propagation is ignored. If *parent_rate comes back with a new rate + * for clk's parent then we propagate up to clk's parent and set it's + * rate. Upward propagation will continue until either a clk does not + * support the CLK_SET_RATE_PARENT flag or .round_rate stops requesting + * changes to clk's parent_rate. + * If there is a failure during upstream + * propagation then clk_set_rate will unwind and restore each clk's rate + * that had been successfully changed. Afterwards a rate change abort + * notification will be propagated downstream, starting from the clk + * that failed. I'm not sure this part still matches the code. + * + * At the end of all of the rate setting, clk_set_rate internally calls + * __clk_recalc_rates and propagates the rate changes downstream, I do not see __clk_recalc_rates is called by clk_set_rate in any way. Regards, Shawn + * starting from the highest clk whose rate was changed. This has the + * added benefit of propagating post-rate change notifiers. + * + * Note that while post-rate change and rate change abort notifications + * are guaranteed to be sent to a clk only once per call to + * clk_set_rate, pre-change notifications will be sent for every clk + * whose rate is changed. Stacking pre-change notifications is noisy + * for the drivers subscribed to them, but this allows drivers to react + * to intermediate clk rate changes up until the point where the final + * rate is achieved at the end of upstream propagation. + * + * Returns 0 on success, -EERROR otherwise. + */ ___ 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
Another trivial comment. But if there is an incremental patch, maybe
consider to include it.
On Thu, Mar 15, 2012 at 11:11:19PM -0700, Mike Turquette wrote:
...
+#ifdef CONFIG_COMMON_CLK_DISABLE_UNUSED
+static int clk_disable_unused(void)
+{
+ struct clk *clk;
+ struct hlist_node *tmp;
+
+ mutex_lock(prepare_lock);
+
+ hlist_for_each_entry(clk, tmp, clk_root_list, child_node)
+ clk_disable_unused_subtree(clk);
+
+ hlist_for_each_entry(clk, tmp, clk_orphan_list, child_node)
+ clk_disable_unused_subtree(clk);
+
+ mutex_unlock(prepare_lock);
+
+ return 0;
+}
+late_initcall(clk_disable_unused);
+#else
+static inline int clk_disable_unused(struct clk *clk) { return 0; }
This #else block seems completely unnecessary to me.
+#endif /* CONFIG_COMMON_CLK_DISABLE_UNUSED */
--
Regards,
Shawn
___
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 Fri, Mar 16, 2012 at 08:23:57PM -0700, Saravana Kannan wrote: ... Hi Mike, I already took a quick look at the v7 series, but I thought this thread has more relevant context for my response. So, responding here. I'm with Sascha on creating a clk_internal/clk_initializer and removing clk_hw. You had asked about the benefits of his suggestion in one of the earlier threads. I'm sure I have told some of these reasons I don't like clk_hw, but repeating my points here for others to chime in. I used to be a huge proponent for using macros for clocks in our internal tree. All the clocks were constructed using macros (you will see it in the history of tree we publish in CAF). They quickly became unreadable when you have several hundreds of clocks. The biggest problem is that you can't quickly look at a clock's macro and figure out what the register offset or bit mask or shift value is. Our eyes/brains aren't meant for quickly parsing the commas and finding the n-th field or even remembering what the n-th field of each macro corresponds to. If it's actually broken out as fields in a struct, it's much easier to read. So, long story short, I think the well-intentioned helper macros will make code quite unreadable. While I second the idea of clk_initializer, using macros is not really the thing to complain. You can save using those macro helpers by calling APIs clk_register_*(). But that does not solve the problem, because those APIs also have a long argument list to fill. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v8 2/2] Regulator: Add Anatop regulator driver
On Mon, Mar 05, 2012 at 11:07:28PM +0800, Ying-Chun Liu (PaulLiu) wrote: ... +MODULE_AUTHOR(Nancy Chen nancy.c...@freescale.com); +MODULE_AUTHOR(Ying-Chun Liu (PaulLiu) paul@linaro.org); I meant something like: MODULE_AUTHOR(Nancy Chen nancy.c...@freescale.com, Ying-Chun Liu (PaulLiu) paul@linaro.org); Otherwise: Acked-by: Shawn Guo shawn@linaro.org +MODULE_DESCRIPTION(ANATOP Regulator driver); +MODULE_LICENSE(GPL v2); -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/2] mfd: Add anatop mfd driver
On Sun, Mar 04, 2012 at 01:39:12AM +0800, Ying-Chun Liu (PaulLiu) wrote:
From: Ying-Chun Liu (PaulLiu) paul@linaro.org
Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
Anatop provides regulators and thermal.
This driver handles the address space and the operation of the mfd device.
Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org
Cc: Samuel Ortiz sa...@linux.intel.com
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Shawn Guo shawn@linaro.org
A few trivial comments below, otherwise
Acked-by: Shawn Guo shawn@linaro.org
Cc: Venu Byravarasu vbyravar...@nvidia.com
Cc: Peter Korsgaard jac...@sunsite.dk
---
drivers/mfd/Kconfig|6 ++
drivers/mfd/Makefile |1 +
drivers/mfd/anatop-mfd.c | 142
include/linux/mfd/anatop.h | 40
4 files changed, 189 insertions(+), 0 deletions(-)
create mode 100644 drivers/mfd/anatop-mfd.c
create mode 100644 include/linux/mfd/anatop.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f147395..78593e8 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
Passage) chip. This chip embeds audio, battery, GPIO, etc.
devices used in Intel Medfield platforms.
+config MFD_ANATOP
+ bool Support for Anatop
It might worth a more descriptive prompt, something like
Support Freescale i.MX on-chip ANATOP controller?
+ depends on SOC_IMX6Q
+ help
+ Select this option to enable Anatop MFD driver.
Ditto, a more descriptive help?
+
endmenu
endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b953bab..8e11060 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o
obj-$(CONFIG_MFD_S5M_CORE) += s5m-core.o s5m-irq.o
+obj-$(CONFIG_MFD_ANATOP) += anatop-mfd.o
diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
new file mode 100644
index 000..0307051
--- /dev/null
+++ b/drivers/mfd/anatop-mfd.c
@@ -0,0 +1,142 @@
+/*
+ * Anatop MFD driver
+ *
+ * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) paul@linaro.org
+ * Copyright (C) 2012 Linaro
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include linux/io.h
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/of.h
+#include linux/of_platform.h
+#include linux/of_address.h
+#include linux/mfd/anatop.h
+
+u32 anatop_get_bits(struct anatop *adata, u32 addr, int bit_shift, int bits)
+{
+ u32 val;
+ int mask;
Shouldn't mask be also u32? Then u32 val, mask;?
+
+ if (bits == 32)
+ mask = ~0;
+ else
+ mask = (1 bits) - 1;
+
+ val = readl(adata-ioreg + addr);
+ val = (val bit_shift) mask;
+
+ return val;
+}
+EXPORT_SYMBOL(anatop_get_bits);
+
+void anatop_set_bits(struct anatop *adata, u32 addr, int bit_shift,
+ int bits, u32 data)
+{
+ u32 val;
+ int mask;
Ditto, with a blank line to be consistent with above function.
+ if (bits == 32)
+ mask = ~0;
+ else
+ mask = (1 bits) - 1;
+
+ spin_lock(adata-reglock);
+ val = readl(adata-ioreg + addr) ~(mask bit_shift);
+ writel((data bit_shift) | val, adata-ioreg);
+ spin_unlock
Re: [PATCH v7 1/2] mfd: Add anatop mfd driver
On Sun, Mar 04, 2012 at 01:39:12AM +0800, Ying-Chun Liu (PaulLiu) wrote:
...
+static int of_anatop_probe(struct platform_device *pdev)
__devinit
+{
+ struct device *dev = pdev-dev;
+ struct device_node *np = dev-of_node;
+ void *ioreg;
+ struct anatop *drvdata;
+
+ ioreg = of_iomap(np, 0);
+ if (!ioreg)
+ return -EINVAL;
+ drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
+ if (!drvdata)
+ return -EINVAL;
+ drvdata-ioreg = ioreg;
+ spin_lock_init(drvdata-reglock);
+ platform_set_drvdata(pdev, drvdata);
+ of_platform_bus_probe(np, of_anatop_subdevice_match, dev);
+
+ return 0;
+}
+
+static int of_anatop_remove(struct platform_device *pdev)
__devexit
+{
+ struct anatop *drvdata;
+ drvdata = platform_get_drvdata(pdev);
+ iounmap(drvdata-ioreg);
+ return 0;
+}
+
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/2] mfd: Add anatop mfd driver
Sorry, one more missing ...
On Sun, Mar 04, 2012 at 01:39:12AM +0800, Ying-Chun Liu (PaulLiu) wrote:
...
+static int of_anatop_probe(struct platform_device *pdev)
+{
+ struct device *dev = pdev-dev;
+ struct device_node *np = dev-of_node;
+ void *ioreg;
+ struct anatop *drvdata;
+
+ ioreg = of_iomap(np, 0);
+ if (!ioreg)
+ return -EINVAL;
+ drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
sizeof(*drvdata) please.
Documentation/CodingStyle, Chapter 14:
The preferred form for passing a size of a struct is the following:
p = kmalloc(sizeof(*p), ...);
The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.
+ if (!drvdata)
+ return -EINVAL;
+ drvdata-ioreg = ioreg;
+ spin_lock_init(drvdata-reglock);
+ platform_set_drvdata(pdev, drvdata);
+ of_platform_bus_probe(np, of_anatop_subdevice_match, dev);
+
+ return 0;
+}
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 1/2] mfd: Add anatop mfd driver
On Thu, Feb 09, 2012 at 04:51:25AM +0800, Ying-Chun Liu (PaulLiu) wrote:
From: Ying-Chun Liu (PaulLiu) paul@linaro.org
Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
Anatop provides regulators and thermal.
This driver handles the address space and the operation of the mfd device.
Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org
Cc: Samuel Ortiz sa...@linux.intel.com
---
drivers/mfd/Kconfig|6 ++
drivers/mfd/Makefile |1 +
drivers/mfd/anatop-mfd.c | 152
include/linux/mfd/anatop.h | 39 +++
4 files changed, 198 insertions(+), 0 deletions(-)
create mode 100644 drivers/mfd/anatop-mfd.c
create mode 100644 include/linux/mfd/anatop.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index cd13e9f..4f71627 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
Passage) chip. This chip embeds audio, battery, GPIO, etc.
devices used in Intel Medfield platforms.
+config MFD_ANATOP
+bool Support for Anatop
+ depends on SOC_IMX6Q
+ help
+ Select this option to enable Anatop MFD driver.
+
endmenu
endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b953bab..8e11060 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o
obj-$(CONFIG_MFD_S5M_CORE) += s5m-core.o s5m-irq.o
+obj-$(CONFIG_MFD_ANATOP) += anatop-mfd.o
diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
new file mode 100644
index 000..58c6054
--- /dev/null
+++ b/drivers/mfd/anatop-mfd.c
@@ -0,0 +1,152 @@
+/*
+ * Anatop MFD driver
+ *
+ * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) paul@linaro.org
+ * Copyright (C) 2012 Linaro
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include linux/io.h
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/of.h
+#include linux/of_platform.h
+#include linux/of_address.h
+#include linux/mfd/anatop.h
+
+static u32 anatop_read(struct anatop *adata, u32 addr, int bit_shift, int
bits)
+{
+ u32 val;
+ int mask;
Nit: it may be nice to put a blank line here.
+ if (bits == 32)
+ mask = 0xff;
Shouldn't it be ~0?
+ else
+ mask = (1 bits) - 1;
+
+ val = ioread32(adata-ioreg+addr);
Why not just readl()? Nit: put space before and after '+'.
+ val = (val bit_shift) mask;
Nit: it may be nice to put a blank line here.
+ return val;
+}
+
+static void anatop_write(struct anatop *adata, u32 addr, int bit_shift,
+ int bits, u32 data)
+{
+ u32 val;
+ int mask;
+ if (bits == 32)
+ mask = 0xff;
+ else
+ mask = (1 bits) - 1;
+
+ val = ioread32(adata-ioreg+addr) ~(mask bit_shift);
+ iowrite32((data bit_shift) | val, adata-ioreg);
Same comments on anatop_read apply on anatop_write here.
+}
+
+static const struct of_device_id of_anatop_regulator_match[] = {
A better naming of of_anatop_regulator_match, since it covers not only
regulator but also thermal as below?
+ {
+ .compatible = fsl,anatop-regulator,
+ },
Nit: since you only have one line here, it could be just:
{ .compatible =
Re: [PATCH v4 2/2] Regulator: Add Anatop regulator driver
On Thu, Feb 09, 2012 at 04:51:26AM +0800, Ying-Chun Liu (PaulLiu) wrote:
From: Ying-Chun Liu (PaulLiu) paul@linaro.org
Anatop is an integrated regulator inside i.MX6 SoC.
There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
This patch adds the Anatop regulator driver.
Signed-off-by: Nancy Chen nancy.c...@freescale.com
Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Liam Girdwood l...@ti.com
---
drivers/regulator/Kconfig |6 +
drivers/regulator/Makefile |1 +
drivers/regulator/anatop-regulator.c | 204
include/linux/regulator/anatop-regulator.h | 49 +++
4 files changed, 260 insertions(+), 0 deletions(-)
create mode 100644 drivers/regulator/anatop-regulator.c
create mode 100644 include/linux/regulator/anatop-regulator.h
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7a61b17..5fbcda2 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -335,5 +335,11 @@ config REGULATOR_AAT2870
If you have a AnalogicTech AAT2870 say Y to enable the
regulator driver.
+config REGULATOR_ANATOP
+ tristate ANATOP LDO regulators
+ depends on MFD_ANATOP
+ help
+ Say y here to support ANATOP LDOs regulators.
+
endif
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 503bac8..8440871 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -48,5 +48,6 @@ obj-$(CONFIG_REGULATOR_AB8500) += ab8500.o
obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
+obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/anatop-regulator.c
b/drivers/regulator/anatop-regulator.c
new file mode 100644
index 000..d800c88
--- /dev/null
+++ b/drivers/regulator/anatop-regulator.c
@@ -0,0 +1,204 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include linux/slab.h
+#include linux/device.h
+#include linux/module.h
+#include linux/err.h
+#include linux/io.h
+#include linux/platform_device.h
+#include linux/regulator/driver.h
+#include linux/regulator/anatop-regulator.h
+#include linux/of.h
+#include linux/of_address.h
+#include linux/regulator/of_regulator.h
+
+static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
+ int max_uV, unsigned *selector)
+{
+ struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+ u32 val, sel;
+ int uv;
+
+ uv = min_uV;
+ pr_debug(%s: uv %d, min %d, max %d\n, __func__,
I would suggest dev_dbg in device driver.
+ uv, anatop_reg-rdata-min_voltage,
+ anatop_reg-rdata-max_voltage);
+
+ if (uv anatop_reg-rdata-min_voltage
+ || uv anatop_reg-rdata-max_voltage) {
+ if (max_uV anatop_reg-rdata-min_voltage)
+ uv = anatop_reg-rdata-min_voltage;
+ else
+ return -EINVAL;
+ }
+
+ if (anatop_reg-rdata-control_reg) {
+ sel = (uv - anatop_reg-rdata-min_voltage) / 25000;
+ val = anatop_reg-rdata-min_bit_val + sel;
+ *selector = sel;
+ pr_debug(%s: calculated val %d\n, __func__, val);
+ anatop_reg-rdata-mfd-write(anatop_reg-rdata-mfd,
+ anatop_reg-rdata-control_reg,
+ anatop_reg-rdata-vol_bit_shift,
+ anatop_reg-rdata-vol_bit_size,
+ val);
+ return 0;
+ } else {
+ return -ENOTSUPP;
+ }
+}
+
+static int anatop_get_voltage_sel(struct regulator_dev *reg)
+{
+ struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+ int selector;
+ struct
Re: [PATCH v4 3/6] clk: introduce the common clock framework
On Thu, Jan 12, 2012 at 04:04:23PM -0800, Saravana Kannan wrote: While the original clk_hw suggestion was well intentioned, it just forces too many unnecessary dereferences and indirection. It also prevents static init of some fields as others have mentioned. Overall, it made the MSM clock code a mess when I tried to convert it to the common clock framework during Linaro Connect Q4 2011. The current off-tree MSM clock code uses a very similar approach to what the original patches that Jeremy sent out did. When Mike sent out the patches that removed clk_hw, the MSM code was much clearer and easier to convert to the common clock framework. I share the same feeling with migrating imx6 clock support to v2 and v3 of this series. (v2 uses clk_hw, and v3 removes clk_hw.) -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V6 2/7] arm/imx: cpufreq: remove loops_per_jiffy recalculate for smp
On Fri, Jan 06, 2012 at 08:56:50AM +0800, Richard Zhao wrote: Hi Sascha Shawn, Could you look and ack the patch? The patch looks good to me. But it really depends on how the patch #1 looks to Russell. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V5 4/7] cpufreq: add clk-reg cpufreq driver
On Wed, Dec 28, 2011 at 12:47:40PM +, Mark Brown wrote: One word. You mean I have to always depends on REGULATOR config, right? Yes. I do not care too much. But it puts the driver on an interesting position, that is it can work without a regulator driver backing the cpu voltage but it has to enable CONFIG_REGULATOR. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V5 4/7] cpufreq: add clk-reg cpufreq driver
On Wed, Dec 28, 2011 at 12:54:21PM +, Mark Brown wrote: On Wed, Dec 28, 2011 at 09:06:20PM +0800, Shawn Guo wrote: On Wed, Dec 28, 2011 at 12:47:40PM +, Mark Brown wrote: One word. You mean I have to always depends on REGULATOR config, right? Yes. I do not care too much. But it puts the driver on an interesting position, that is it can work without a regulator driver backing the cpu voltage but it has to enable CONFIG_REGULATOR. Well, the other option is ifdefs in your driver. I'm much more comfortable with this oddness than ifdefs :) -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3] Regulator: Add Anatop regulator driver
On Tue, Dec 27, 2011 at 06:16:34PM +0800, Ying-Chun Liu (PaulLiu) wrote:
From: Ying-Chun Liu (PaulLiu) paul@linaro.org
Anatop is an integrated regulator inside i.MX6 SoC.
There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
This patch adds the Anatop regulator driver.
Signed-off-by: Nancy Chen nancy.c...@freescale.com
Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Liam Girdwood l...@ti.com
---
drivers/regulator/Kconfig |6 +
drivers/regulator/Makefile |1 +
drivers/regulator/anatop-regulator.c | 214
include/linux/regulator/anatop-regulator.h | 63
4 files changed, 284 insertions(+), 0 deletions(-)
create mode 100644 drivers/regulator/anatop-regulator.c
create mode 100644 include/linux/regulator/anatop-regulator.h
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 9713b1b..fc22b8d 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -327,5 +327,11 @@ config REGULATOR_AAT2870
If you have a AnalogicTech AAT2870 say Y to enable the
regulator driver.
+config REGULATOR_ANATOP
+ tristate ANATOP LDO regulators
+ depends on SOC_IMX6
There is no symbol 'SOC_IMX6'. Instead, it's 'SOC_IMX6Q'.
[...]
+int anatop_regulator_probe(struct platform_device *pdev)
+{
+ struct regulator_desc *rdesc;
+ struct regulator_dev *rdev;
+ struct anatop_regulator *sreg;
+ struct regulator_init_data *initdata;
+
+ initdata = pdev-dev.platform_data;
It seems that the driver only gets probed in non-dt way. But imx6q
only supports DT. How does this driver work on imx6q?
PS. The regulator DT binding has been available on -next, so I do not
understand why you are still getting regulator_init_data from
platform_data rather than device tree.
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V5 4/7] cpufreq: add clk-reg cpufreq driver
Hi Richard,
On Tue, Dec 27, 2011 at 04:24:19PM +0800, Richard Zhao wrote:
The driver get cpu operation point table from device tree cpu0 node,
and adjusts operating points using clk and regulator APIs.
It support single core and multi-core ARM SoCs. But currently it assume
all cores share the same frequency and voltage.
Signed-off-by: Richard Zhao richard.z...@linaro.org
Reviewed-by: Jamie Iles ja...@jamieiles.com
Reviewed-by: Mark Brown broo...@opensource.wolfsonmicro.com
---
.../devicetree/bindings/cpufreq/clk-reg-cpufreq| 21 ++
drivers/cpufreq/Kconfig| 10 +
drivers/cpufreq/Makefile |2 +
drivers/cpufreq/clk-reg-cpufreq.c | 302
4 files changed, 335 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/cpufreq/clk-reg-cpufreq
create mode 100644 drivers/cpufreq/clk-reg-cpufreq.c
[...]
+static struct cpufreq_driver clk_reg_cpufreq_driver = {
+ .flags = CPUFREQ_STICKY,
+ .verify = clk_reg_verify_speed,
+ .target = clk_reg_set_target,
+ .get = clk_reg_get_speed,
+ .init = clk_reg_cpufreq_init,
+ .exit = clk_reg_cpufreq_exit,
+ .name = clk-reg,
+};
+
+static u32 max_freq = UINT_MAX / 1000; /* kHz */
+module_param(max_freq, uint, 0);
+MODULE_PARM_DESC(max_freq, max cpu frequency in unit of kHz);
+
Have you tried to pass this param from kernel cmdline? What's the
syntax if we want to pass a 800 MHz max_freq?
And I played this driver on imx6q with pm-qa [1] cpufreq test suit from
Linaro PMWG.
### cpufreq_01:
### test the cpufreq framework is available for frequency
###
https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/QA/Scripts#cpufreq_01
###
cpufreq_01.0/cpu0: checking 'scaling_available_frequencies' exists... fail
cpufreq_01.0/cpu1: checking 'scaling_available_frequencies' exists... fail
cpufreq_01.0/cpu2: checking 'scaling_available_frequencies' exists... fail
cpufreq_01.0/cpu3: checking 'scaling_available_frequencies' exists... fail
### cpufreq_05:
### test 'ondemand' and 'conservative' trigger correctly the configuration
directory
###
https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/QA/Scripts#cpufreq_05
###
cpufreq_05.0: checking 'ondemand' directory exists... pass
cpufreq_05.1: checking 'conservative' directory exists... pass
cpufreq_05.2: checking 'ondemand' directory is not there... pass
cpufreq_05.3: checking 'conservative' directory is not there... pass
cpufreq_05.4: checking 'ondemand' directory exists... fail
cpufreq_05.5: checking 'conservative' directory exists... pass
The cpufreq_01 can be easily fixed with the following change.
8-
@@ -146,6 +150,11 @@ static int clk_reg_cpufreq_exit(struct cpufreq_policy
*policy)
return 0;
}
+static struct freq_attr *clk_reg_cpufreq_attr[] = {
+ cpufreq_freq_attr_scaling_available_freqs,
+ NULL,
+};
+
static struct cpufreq_driver clk_reg_cpufreq_driver = {
.flags = CPUFREQ_STICKY,
.verify = clk_reg_verify_speed,
@@ -153,10 +162,15 @@ static struct cpufreq_driver clk_reg_cpufreq_driver = {
.get = clk_reg_get_speed,
.init = clk_reg_cpufreq_init,
.exit = clk_reg_cpufreq_exit,
+ .attr = clk_reg_cpufreq_attr,
.name = clk-reg,
};
-8
And I have not looked into the second one deeply, but maybe you
want to :)
--
Regards,
Shawn
[1] git://git.linaro.org/people/dlezcano/pm-qa.git
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V5 4/7] cpufreq: add clk-reg cpufreq driver
On Wed, Dec 28, 2011 at 09:24:05AM +0800, Richard Zhao wrote: Have you tried to pass this param from kernel cmdline? What's the syntax if we want to pass a 800 MHz max_freq? clk-reg-cpufreq.max_freq=80 Thanks. I was mistaken on the module name. ### cpufreq_05: ### test 'ondemand' and 'conservative' trigger correctly the configuration directory ### https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/QA/Scripts#cpufreq_05 ### cpufreq_05.0: checking 'ondemand' directory exists... pass cpufreq_05.1: checking 'conservative' directory exists... pass cpufreq_05.2: checking 'ondemand' directory is not there... pass cpufreq_05.3: checking 'conservative' directory is not there... pass cpufreq_05.4: checking 'ondemand' directory exists... fail cpufreq_05.5: checking 'conservative' directory exists... pass I past fail part script here: switch_ondemand cpu0 switch_conservative cpu1 check 'ondemand' directory exists test -d $CPU_PATH/cpufreq/ondemand check 'conservative' directory exists test -d $CPU_PATH/cpufreq/conservative This driver assume all cpu cores to share the same freq and voltage. The affected cpu is all other cpus. They also share one single governor. The test case does not suit this driver and not for most arm multi-core cpus I guess. Then this is the feedback that Linaro PMWG wants to have, I guess. Here is my tag on this patch. Acked-by: Shawn Guo shawn@linaro.org -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V5 4/7] cpufreq: add clk-reg cpufreq driver
On Wed, Dec 28, 2011 at 10:01:13AM +0800, Shawn Guo wrote: Here is my tag on this patch. Acked-by: Shawn Guo shawn@linaro.org For record, this tag is only valid with the following conditions. * Fix the failure of pm-qa case cpufreq_01 * Fix the failure of module build * Remove the dependency on REGULATOR since a set of empty functions have already been provided for !REGULATOR build. (regulator_is_supported_voltage and regulator_set_voltage_time to be added) -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC V1 2/4] dts/imx6q: add cpufreq property
Hi Richard,
Whenever we invent some new device tree binding support, we need to
Cc devicetree-disc...@lists.ozlabs.org (Cc-ed).
On Thu, Dec 15, 2011 at 07:16:36PM +0800, Richard Zhao wrote:
Signed-off-by: Richard Zhao richard.z...@linaro.org
---
arch/arm/boot/dts/imx6q.dtsi |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 263e8f3..9e9943b 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -29,6 +29,8 @@
compatible = arm,cortex-a9;
reg = 0;
next-level-cache = L2;
+ cpu_freqs = 99600 79200 39600 19800;
+ cpu_volts = 1225000 110 95 85;
For dt property name, we use '-' rather than '_'.
And I'm not sure this is correct, since these data obviously does not
belong to arm,cortex-a9.
Regards,
Shawn
};
cpu@1 {
--
1.7.5.4
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 2/5] Documentation: common clk API
On Wed, Nov 23, 2011 at 12:33:47PM -0800, Turquette, Mike wrote:
On Tue, Nov 22, 2011 at 6:03 PM, Saravana Kannan skan...@codeaurora.org
wrote:
On 11/21/2011 05:40 PM, Mike Turquette wrote:
[...]
+is modified slightly for brevity:
+
+struct clk {
+ const char *name;
+ const struct clk_hw_ops *ops;
No strong opinion, but can we call this clk_ops for brevity?
I prefer clk_hw_ops, as it clearly delineates that these operations
know hardware-specific details.
I tend to agree with Saravana for brevity. Looking at clk-basic.c,
I do not think it's necessary to encode 'hw' in naming of clk_hw_fixed,
clk_hw_gate and any clocks wrapping 'struct clk' in there. For
example, naming like clk_dummy, clk_imx seems brevity and clear,
and we do not need clk_hw_dummy and clk_hw_imx necessarily.
[...]
+
+clk_set_rate - Attempts to change the clk rate to the one specified.
+Depending on a variety of common flags it may fail to maintain system
+stability or result in a variety of other clk rates changing.
I'm not sure if this is intentional or if it's a mistake in phrasing it.
IMHO, the rates of other clocks that are actually made available to device
drivers should not be changed. This call might trigger rate changes inside
the tree to accommodate the request from various children, but should not
affect the rate of the leaf nodes.
Can you please clarify the intention and/or the wording?
Setting the flag CLK_GATE_SET_RATE tells clk_set_rate not to change
the rate if the clk is enabled. This policy is not enforced
abritrarily: you don't have to set the flag on your clk. I'll update
the doc to make explicit mention of this flag.
I guess the concern is not about the flag but the result of clk_set_rate
that might change a variety of other clocks, while Saravana said it
should not. And I agree with Saravana.
+clk_set_rate deserves a special mention because it is more complex than
+the other operations. There are three key concepts to the common
+clk_set_rate implementation:
+
+1) recursively traversing up the clk tree and changing clk rates, one
+parent at a time, if each clk allows it
+2) failing to change rate if the clk is enabled and must only change
+rates while disabled
Is this really true? Based on a quick glance at the other patches, it
doesn't look it disallows set rate if the clock is enabled. Which is
correct, because clock rates can be change while they are enabled (I'm
referring to leaf clocks) as long as the hardware supports doing it
correctly. So, this needs rewording? I'm guessing you are trying to say
that you can't change the parent rate if it has multiple enabled child
clocks running off of it and one of them wants to cause a parent rate
change? I'm not sure if even that should be enforced in the common code --
doesn't look like you do either.
Same as my answer above. There is a flag which allows you to control
this behavior.
On the contrary, I have clocks on mxs platform which can be set rate
only when they are enabled, while there are users call clk_set_rate
when the clock is not enabled yet. Do we need another flag
CLK_ON_SET_RATE for this type of clocks?
I'm unsure that clk API users will all agree with the use of the flags.
From the code, the clock framework will make the call fail if users
attempt to clk_set_rate an enabled clock with flag CLK_GATE_SET_RATE.
And clk API users might argue that they do not (need to) know this
clock details, and it's clock itself (clock framework or/and clock
driver) who should handle this type of details.
+2) using clk rate change notifiers to allow devices to handle dynamic
Must be 3)
Haha good catch.
+rate changes for clks which do support changing rates while enabled
Again, I guess this applies to the other clock. Not the one for which
clk_set_rate() is being called.
This applies to ANY clk which has the flag set and is called by
__clk_set_rate (which may be called many times in a recursive path).
+clk_set_rate(C, 26MHz);
+ __clk_set_rate(C, 26MHz);
+ clk-round_rate(C, 26MHz, *parent_rate);
+ [ round_rate returns 50MHz ]
+ [parent_rate is 52MHz ]
+ clk-set_rate(C, 50Mhz);
+ [ clk C is set to 50MHz, which sets divider to 2 ]
+ __clk_set_rate(clk-parent, parent_rate);
+ clk-round_rate(B, 52MHz, *parent_rate);
+ [ round_rate returns 100MHz ]
+ [parent_rate is 104MHz ]
+ clk-set_rate(B, 100MHz);
+ [ clk B stays at 100MHz, divider stays at 2 ]
+ __clk_set_rate(clk-parent, parent_rate);
+ [ round_rate returns 104MHz ]
+ [parent_rate is ignored ]
+
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On Mon, Nov 21, 2011 at 05:40:45PM -0800, Mike Turquette wrote:
[...]
+/**
+ * DOC: Using the CLK_PARENT_SET_RATE 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_GATE_SET_RATE flag on any
+ * clk where you also set the CLK_PARENT_SET_RATE flag
Eh, this is how flag CLK_GATE_SET_RATE is born? Does that mean to make
the call succeed all the clocks on the propagating path need to be gated
before clk_set_rate gets called?
+ */
+struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ struct clk *fail_clk = NULL;
+ int ret;
+ 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_GATE_SET_RATE) 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, rate, parent_new_rate);
+
+ /* FIXME propagate pre-rate change notification here */
+ /* XXX note that pre-rate change notifications will stack */
+
+ /* change the rate of this clk */
+ ret = clk-ops-set_rate(clk, new_rate);
Yes, right here, the clock gets set to some unexpected rate, since the
parent clock has not been set to the target rate yet at this point.
+ if (ret)
+ return clk;
+
+ /*
+ * change the rate of the parent clk if necessary
+ *
+ * hitting the nested 'if' path implies we have hit a .set_rate
+ * failure somewhere upstream while propagating __clk_set_rate
+ * up the clk tree. roll back the clk rates one by one and
+ * return the pointer to the clk that failed. clk_set_rate will
+ * use the pointer to propagate a rate-change abort notifier
+ * from the highest point.
+ */
+ if ((clk-flags CLK_PARENT_SET_RATE) parent_new_rate) {
+ parent_old_rate = clk-parent-rate;
+ fail_clk = __clk_set_rate(clk-parent, parent_new_rate);
+
+ /* roll back changes if parent rate change failed */
+ if (fail_clk) {
+ pr_warn(%s: failed to set parent %s rate to %lu\n,
+ __func__, fail_clk-name,
+ parent_new_rate);
+ clk-ops-set_rate(clk, old_rate);
+ }
+ return fail_clk;
+ }
+
+ /*
+ * set clk's rate recalculate the rates of clk's children
+ *
+ * hitting this path implies we have successfully finished
+ * propagating recursive calls to __clk_set_rate up the clk tree
+ * (if necessary) and it is safe to propagate clk_recalc_rates and
+ * post-rate change notifiers down the clk tree from this point.
+ */
+ clk-rate = new_rate;
+ /* FIXME propagate post-rate change notifier for only this clk */
+ hlist_for_each_entry(child, tmp, clk-children, child_node)
+ clk_recalc_rates(child);
+
+ return fail_clk;
+}
[...]
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 7213b52..3b0eb3f 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -3,6 +3,8 @@
*
* Copyright (C) 2004 ARM Limited.
* Written by Deep Blue Solutions Limited.
+ * Copyright (c) 2010-2011 Jeremy Kerr jeremy.k...@canonical.com
+ * Copyright (C) 2011 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
@@ -13,17 +15,134 @@
#include linux/kernel.h
+#include linux/kernel.h
Eh, why adding a duplication?
+#include linux/errno.h
+
struct device;
+struct clk;
Do you really need this?
[...]
+struct clk_hw_ops {
+ int (*prepare)(struct clk *clk);
+ void(*unprepare)(struct clk *clk);
+ int (*enable)(struct clk *clk);
+ void(*disable)(struct clk *clk);
+ unsigned long (*recalc_rate)(struct clk *clk);
+ long(*round_rate)(struct clk *clk, unsigned long,
The return type seems interesting. If we intend to return a rate, it
should be 'unsigned long' rather than 'long'. I'm just curious about
the possible reason behind that.
+ unsigned long *);
+ int (*set_parent)(struct clk *clk, struct clk *);
+ struct clk *(*get_parent)(struct clk *clk);
+ int (*set_rate)(struct clk *clk, unsigned long);
Nit: would it be
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
On Mon, Nov 21, 2011 at 05:40:46PM -0800, Mike Turquette wrote:
Many platforms support simple gateable clks and fixed-rate clks that
should not be re-implemented by every platform.
This patch introduces a gateable clk with a common programming model of
gate control via a write of 1 bit to a register. Both set-to-enable and
clear-to-enable are supported.
Also introduced is a fixed-rate clk which has no reprogrammable aspects.
The purpose of both types of clks is documented in drivers/clk/basic.c.
What I have seen is drivers/clk/clk-basic.c.
TODO: add support for a simple divider, simple mux and a dummy clk for
stubbing out platform support.
Based on original patch by Jeremy Kerr contribution by Jamie Iles.
Signed-off-by: Mike Turquette mturque...@linaro.org
---
drivers/clk/Kconfig |7 ++
drivers/clk/Makefile|5 +-
drivers/clk/clk-basic.c | 208
+++
include/linux/clk.h | 35
4 files changed, 253 insertions(+), 2 deletions(-)
create mode 100644 drivers/clk/clk-basic.c
[...]
+int clk_register_gate(struct device *dev, const char *name, unsigned long
flags,
+ struct clk *fixed_parent, void __iomem *reg, u8 bit_idx,
+ int set_to_enable)
+{
+ struct clk_hw_gate *gclk;
+ struct clk *clk;
+
+ gclk = kmalloc(sizeof(struct clk_hw_gate), GFP_KERNEL);
+
+ if (!gclk) {
+ pr_err(%s: could not allocate gated clk\n, __func__);
+ return -ENOMEM;
+ }
+
+ clk = gclk-clk;
+
+ /* struct clk_hw_gate assignments */
+ gclk-fixed_parent = fixed_parent;
+ gclk-reg = reg;
+ gclk-bit_idx = bit_idx;
+
+ /* struct clk assignments */
+ clk-name = name;
+ clk-flags = flags;
+
+ if (set_to_enable)
+ clk-ops = clk_hw_gate_set_enable_ops;
+ else
+ clk-ops = clk_hw_gate_set_disable_ops;
+
+ clk_init(NULL, clk);
+
+ return 0;
The device tree support needs to get this 'struct clk *', so we may
want to have all these registering functions return the 'clk'.
+}
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
One comment was missed.
On Mon, Nov 21, 2011 at 05:40:46PM -0800, Mike Turquette wrote:
[...]
+struct clk_hw_ops clk_hw_gate_set_enable_ops = {
const?
+ .enable = clk_hw_gate_enable_set,
+ .disable = clk_hw_gate_disable_clear,
+ .recalc_rate = clk_hw_gate_recalc_rate,
+ .get_parent = clk_hw_gate_get_parent,
+};
+EXPORT_SYMBOL_GPL(clk_hw_gate_set_enable_ops);
+
+static int clk_hw_gate_enable_clear(struct clk *clk)
+{
+ clk_hw_gate_clear_bit(clk);
+
+ return 0;
+}
+
+static void clk_hw_gate_disable_set(struct clk *clk)
+{
+ clk_hw_gate_set_bit(clk);
+}
+
+struct clk_hw_ops clk_hw_gate_set_disable_ops = {
ditto
Regards,
Shawn
+ .enable = clk_hw_gate_enable_clear,
+ .disable = clk_hw_gate_disable_set,
+ .recalc_rate = clk_hw_gate_recalc_rate,
+ .get_parent = clk_hw_gate_get_parent,
+};
+EXPORT_SYMBOL_GPL(clk_hw_gate_set_disable_ops);
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 1/5] clk: Kconfig: add entry for HAVE_CLK_PREPARE
On Mon, Nov 21, 2011 at 05:40:43PM -0800, Mike Turquette wrote: The common clk framework provides clk_prepare and clk_unprepare implementations. Create an entry for HAVE_CLK_PREPARE so that GENERIC_CLK can select it. Signed-off-by: Mike Turquette mturque...@linaro.org --- Acked-by: Shawn Guo shawn@linaro.org Regards, Shawn drivers/clk/Kconfig |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 3530927..7a9899bd 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -5,3 +5,6 @@ config CLKDEV_LOOKUP config HAVE_MACH_CLKDEV bool + +config HAVE_CLK_PREPARE + bool -- 1.7.4.1 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 0/5] common clk framework
On Mon, Nov 21, 2011 at 05:40:42PM -0800, Mike Turquette wrote: [...] .the most notable change is the removal of struct clk_hw. Happy to see that. This extra layer of abstraction is only necessary if we want hide the definition of struct clk from platform code. Many developers expressed the need to know some details of the generic struct clk in the platform layer, and rightly so. Now struct clk is defined in include/linux/clk.h, protected by #ifdef CONFIG_GENERIC_CLK. .flags have been introduced to struct clk, with several of them defined and used in the common code. These flags protect against changing clk rates or switching the clk parent while that clk is enabled; another flag is used to signal to clk_set_rate that it should ask the parent to change it's rate too. .speaking of which, clk_set_rate has been overhauled and is now recursive. *collective groan*. clk_set_rate is still simple for the common case of simply setting a single clk's rate. But if your clk has the CLK_PARENT_SET_RATE flag and the .round_rate callback recommends changing the parent rate, then clk_set_rate will recurse upwards to the parent and try it all over again. In the event of a failure everything unwinds and all the clks go out for drinks. I smell that this will be the part which makes the whole series risky of being accepted in a desired time frame, with saying rate change notifications are still missing for now. .clk_register has been replaced by clk_init, which does NOT allocate memory for you. Platforms should allocate their own clk_hw_whatever structure which contains struct clk. clk_init is still necessary to initialize struct clk internals. clk_init also accepts struct device *dev as an argument, but does nothing with it. This is in anticipation of device tree support. I would say that we do not necessarily need to have 'struct device' for each clk (for imx6 example, we have 110 clks, and I heard some omap support has 225 clks?). The device tree support can work out everything it needs from the 'struct device_node', which can be a clock blob constraining multiple clks (thanks to 'clock-cells'). That said you may want to add the 'dev' argument for other reasons, but I never used it when converting imx6 clock to device tree. .Documentation! I'm sure somebody reads it. .sysfs support. Visualize your clk tree at /sys/clk! Where would be a better place to put the clk tree besides the root of /sys/? When a consensus on this is reached I'll submit the proper changes to Documentation/ABI/testing/. What's missing? .per tree locking. I implemented this at the Linaro Connect conference but the implementation was unpopular, so it didn't make the cut. There needs to be better understanding of everyone's needs for this to work. .rate change notifications. I simply didn't want to delay getting these patches to the list any longer, so the notifiers didn't make it in. I'll submit them to the list soon, or roll them into the V4 patchset. There are comments in the clk API definitions for where PRECHANGE, POSTCHANGE and ABORT propagation will go. .basic mux clk, divider and dummy clk implementations. I think others have some code lying around to implement these, so I left them out. .device tree support. I haven't looked much at the on-going discussions on the dt clk bindings. How compatible (or not) are the device tree clk bindings and the way these patches want to initialize clks? I have just converted imx6 clock to device tree based on this series and Grant's of-clk series with one minor change on clk-basic which technically is not part of the core support. So I would say, do not worry, it's perfectly compatible with device tree :) .what is the overlap between common clk and clkdev? We're essentially tracking the clks in two places (common clk's tree and clkdevs's list), which feels a bit wasteful. I do not see the overlap between these two. The clkdev is nothing but a list maintaining the mapping between necessary 'struct clk' and its consumer's 'struct dev'. It has no clock tree topology, and common clk's tree has no that mapping. -- Regards, Shawn ___ 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
Hi Mike,
Some random comments/nits ...
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
+struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
+ const char *name)
+{
+ struct clk *clk;
+
+ clk = kzalloc(sizeof(*clk), GFP_KERNEL);
+ if (!clk)
+ return NULL;
+
+ INIT_HLIST_HEAD(clk-children);
+ INIT_HLIST_NODE(clk-child_node);
+
The children and child_node are introduced in patch #2, and should not
be used in patch #1.
+ clk-name = name;
+ clk-ops = ops;
+ clk-hw = hw;
+ hw-clk = clk;
+
+ /* Query the hardware for parent and initial rate */
+
+ if (clk-ops-get_parent)
+ /* We don't to lock against prepare/enable here, as
+ * the clock is not yet accessible from anywhere */
/*
* Multiple line comments
*/
+ clk-parent = clk-ops-get_parent(clk-hw);
+
+ if (clk-ops-recalc_rate)
+ clk-rate = clk-ops-recalc_rate(clk-hw);
+
+
One blank line is good enough.
+ return clk;
+}
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 2/7] clk: Implement clk_set_rate
On Thu, Sep 22, 2011 at 03:26:57PM -0700, Mike Turquette wrote: From: Jeremy Kerr jeremy.k...@canonical.com Implement clk_set_rate by adding a set_rate callback to clk_hw_ops. Rates are propagated down the clock tree and recalculated. Also adds a flag for signaling that parents must change rates to achieve the desired frequency (upstream propagation). TODO: Upstream propagation is not yet implemented. Device pre-change and post-change notifications are not implemented, but are marked up as FIXME comments. Signed-off-by: Jeremy Kerr jeremy.k...@canonical.com Signed-off-by: Mark Brown broo...@opensource.wolfsonmicro.com Signed-off-by: Mike Turquette mturque...@ti.com --- Changes since v1: Remove upstream propagation (for now) Rename CLK_SET_RATE_PROPAGATE to CLK_PARENT_RATE_CHANGE [...] + * @set_rate Change the rate of this clock. If this callback returns + * CLK_SET_RATE_PROPAGATE, the rate change will be propagated to s/CLK_SET_RATE_PROPAGATE/CLK_PARENT_RATE_CHANGE, as suggested by your change log above? + * the parent clock (which may propagate again). The requested + * rate of the parent is passed back from the callback in the + * second 'unsigned long *' argument. + * -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 3/7] clk: Add fixed-rate clock
On Thu, Sep 22, 2011 at 03:26:58PM -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: Mike Turquette mturque...@ti.com
---
Changes since v1:
Add copyright header
drivers/clk/Kconfig |4
drivers/clk/Makefile|1 +
drivers/clk/clk-fixed.c | 24
include/linux/clk.h | 14 ++
4 files changed, 43 insertions(+), 0 deletions(-)
create mode 100644 drivers/clk/clk-fixed.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index c53ed59..d8313d7 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -8,3 +8,7 @@ config HAVE_MACH_CLKDEV
config GENERIC_CLK
bool
+
+config GENERIC_CLK_FIXED
+ bool
+ depends on GENERIC_CLK
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 570d5b9..9a3325a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -1,3 +1,4 @@
obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o
obj-$(CONFIG_GENERIC_CLK)+= clk.o
+obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o
diff --git a/drivers/clk/clk-fixed.c b/drivers/clk/clk-fixed.c
new file mode 100644
index 000..956fb9a
--- /dev/null
+++ b/drivers/clk/clk-fixed.c
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd jeremy.k...@canonical.com
+ *
+ * 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.
+ *
+ * Simple fixed-rate clock implementation
+ */
+
+#include linux/clk.h
+#include linux/module.h
+
+#define to_clk_fixed(c) container_of(c, struct clk_hw_fixed, hw)
+
+static unsigned long clk_fixed_recalc_rate(struct clk_hw *hw)
+{
+ return to_clk_fixed(hw)-rate;
+}
+
+struct clk_hw_ops clk_fixed_ops = {
+ .recalc_rate = clk_fixed_recalc_rate,
+};
+EXPORT_SYMBOL_GPL(clk_fixed_ops);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 0d2cd5e..1903dd8 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -110,6 +110,20 @@ int clk_prepare(struct clk *clk);
*/
void clk_unprepare(struct clk *clk);
+/* Base clock implementations. Platform clock implementations can use these
+ * directly, or 'subclass' as approprate */
+
/*
* Multiple lines comments
*/
Regards,
Shawn
+#ifdef CONFIG_GENERIC_CLK_FIXED
+
+struct clk_hw_fixed {
+ struct clk_hw hw;
+ unsigned long rate;
+};
+
+extern struct clk_hw_ops clk_fixed_ops;
+
+#endif /* CONFIG_GENERIC_CLK_FIXED */
+
/**
* clk_register - register and initialize a new clock
*
--
1.7.4.1
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 2/2] pinctrl: add a generic control interface
On Thu, Oct 20, 2011 at 05:17:08PM +0800, Barry Song wrote:
+enum pin_config_param {
+ PIN_CONFIG_BIAS_UNKNOWN,
+ PIN_CONFIG_BIAS_FLOAT,
+ PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
+ PIN_CONFIG_BIAS_PULL_UP,
+ PIN_CONFIG_BIAS_PULL_DOWN,
+ PIN_CONFIG_BIAS_HIGH,
+ PIN_CONFIG_BIAS_GROUND,
+ PIN_CONFIG_DRIVE_UNKNOWN,
+ PIN_CONFIG_DRIVE_PUSH_PULL,
+ PIN_CONFIG_DRIVE_OPEN_DRAIN,
+ PIN_CONFIG_DRIVE_OPEN_SOURCE,
+ PIN_CONFIG_DRIVE_OFF,
+ PIN_CONFIG_INPUT_SCHMITT,
+ PIN_CONFIG_SLEW_RATE_RISING,
+ PIN_CONFIG_SLEW_RATE_FALLING,
+ PIN_CONFIG_LOAD_CAPACITANCE,
+ PIN_CONFIG_WAKEUP_ENABLE,
+ PIN_CONFIG_END,
+};
+
IMO, trying to enumerate all possible pin_config options is just to
make everyone's life harder. Firstly, this enumeration is far from
completion, for imx6 iomuxc example, we have 3 options for pull-up,
22, 47 and 100 kOhm, and 7 options for driver-strength, 34, 40, 48,
60, 80, 120, and 240 Ohm. It's just so hard to make this enumeration
complete. Secondly, defining this param as enum requires the API
user to call the function multiple times to configure one pin. For
example, if I want to configure pin_foo as slow-slew, open-drain,
100 kOhm pull-up and 120 Ohm driver-strength, I will have to call
pin_config(pctldev, pin_foo, ...) 4 times to achieve that.
I like Stephen's idea about having 'u32 param' and let pinctrl drivers
to encode/decode this u32 for their pinctrl controller. It makes
people's life much easier.
A multifunctional API is actually a bad and hard-to-use API. i agree
we can make param u32 instead of enum and let specific driver
customizes the param by itself.
But if there are some common params, for example, PULL, OC/OD,
WAKEUP_ENABLE, which almost all platforms need, why don't we make
them have common definitions like:
#define PIN_CONFIG_PULL 0
#define PIN_CONFIG_OPEN_DRAIN1
#define PIN_CONFIG_USER5 (in case)
if one platform has config not in the up list, then:
#define PIN_CONFIG_TERGA_XXXPIN_CONFIG_USER
#define PIN_CONFIG_TERGA_YYY(PIN_CONFIG_USER + 1)
without the common definition from PIN_CONFIG_PULL to
PIN_CONFIG_USER, many platforms will need to definite them repeatedly.
that is what we hate.
I prefer to completely leave the encoding of this 'u32 param' to pinctrl
drivers, so that they can map the bit definition of 'param' to the
controller register as much as they can. On imx, we have one register
accommodating all pin configuration options for one pin, so we can
write 'param' directly into register with exactly same bit definition
as register.
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 2/2] pinctrl: add a generic control interface
On Wed, Oct 19, 2011 at 06:21:14PM +0200, Linus Walleij wrote:
From: Linus Walleij linus.wall...@linaro.org
This add per-pin and per-group pin control interfaces for biasing,
driving and other such electronic properties. The intention is
clearly to enumerate all things you can do with pins, hoping that
these are enumerable.
Signed-off-by: Linus Walleij linus.wall...@linaro.org
---
Documentation/pinctrl.txt | 55 ++-
drivers/pinctrl/core.c | 69
include/linux/pinctrl/pinctrl.h | 112
++-
3 files changed, 232 insertions(+), 4 deletions(-)
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index b04cb7d..328adb7 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -7,8 +7,6 @@ This subsystem deals with:
- Multiplexing of pins, pads, fingers (etc) see below for details
-The intention is to also deal with:
-
- Software-controlled biasing and driving mode specific pins, such as
pull-up/down, open drain etc, load capacitance configuration when
controlled
by software, etc.
@@ -193,6 +191,59 @@ structure, for example specific register ranges
associated with each group
and so on.
+Pin configuration
+=
+
+Pins can sometimes be software-configured in an tremendous amount of ways,
+mostly related to their electronic properties when used as inputs or outputs.
+For example you may be able to make an output pin high impedance, or
tristate
+meaning it is effectively disconnected. You may be able to connect an input
pin
+to VDD or GND using a certain resistor value - pull up and pull down - so
that
+the pin has a stable value when nothing is driving the rail it is connected
+to, or when it's unconnected.
+
+The pin control system supports an interface partly abstracting these
+properties while leaving the details to the pin control driver.
+
+For example, a driver can do this:
+
+ret = pin_config(128, PIN_CONFIG_BIAS_PULL_UP, 10);
+
+To pull up a pin to VDD with a 100KOhm resistor. The driver implements
+callbacks for changing pin configuration in the pin controller ops like this:
+
+int foo_pin_config (struct pinctrl_dev *pctldev,
+ unsigned pin,
+ enum pin_config_param param,
+ unsigned long data)
+{
+ switch (param) {
+ case PIN_CONFIG_BIAS_PULL_UP:
+ ...
+}
+
+int foo_pin_config_group (struct pinctrl_dev *pctldev,
+ unsigned selector,
+ enum pin_config_param param,
+ unsigned long data)
+{
+ ...
+}
+
+static struct pinctrl_ops foo_pctrl_ops = {
+ ...
+ pin_config = foo_pin_config,
+ pin_config_group = foo_pin_config_group,
+};
+
+Since some controllers have special logic for handling entire groups of pins
+they can exploit the special whole-group pin control function. The
+pin_config_group() callback is allowed to return the error code -EAGAIN,
+for groups it does not want to handle, or if it just wants to do some
+group-level handling in which case each individual pin will be handled by
+separate pin_config() calls as well.
+
+
Interaction with the GPIO subsystem
===
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 478a002..913f760 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -315,6 +315,75 @@ int pinctrl_get_group_selector(struct pinctrl_dev
*pctldev,
return -EINVAL;
}
+int pin_config(struct pinctrl_dev *pctldev, int pin,
+enum pin_config_param param, unsigned long data)
+{
+ const struct pinctrl_ops *ops = pctldev-desc-pctlops;
+
+ if (!ops-pin_config) {
+ dev_err(pctldev-dev, cannot configure pin, missing
+ config function in driver\n);
+ return -EINVAL;
+ }
+
+ return ops-pin_config(pctldev, pin, param, data);
+}
+
+int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
+ enum pin_config_param param, unsigned long data)
+{
+ const struct pinctrl_ops *ops = pctldev-desc-pctlops;
+ int selector;
+ unsigned *pins;
+ unsigned num_pins;
+ int ret;
+ int i;
+
+ if (!ops-pin_config_group !ops-pin_config) {
+ dev_err(pctldev-dev, cannot configure pin group, missing
+ config function in driver\n);
+ return -EINVAL;
+ }
+
+ selector = pinctrl_get_group_selector(pctldev, pin_group);
+ if (selector 0)
+ return selector;
+
+ /*
+ * If the pin controller supports handling entire groups we use that
+ * capability.
+ */
+ if (ops-pin_config_group) {
+ ret = ops-pin_config_group(pctldev, selector, param, data);
+ /*
+ * If the pin
Re: pinctrl_config APIs, and other pinmux questions
On Mon, Oct 17, 2011 at 08:51:11AM -0700, Stephen Warren wrote: Shawn Guo wrote at Friday, October 14, 2011 9:12 PM: On Fri, Oct 14, 2011 at 08:53:33AM -0700, Stephen Warren wrote: ... Having the driver expose a list of all possible combinations of pin configurations seems impractical, for the same reason as I objected to the original proposal for how the driver listed functions; there are simply far too many pin config parameters and legal value to list the entire set of combinations. I did not mean to list the entire set of combinations. For given function, the applicable number of config should be very limited. For most cases, it could be just one. For imx6q usdhc example, it's 3, for 50M, 100M and 200M SD bus clock cases. Shawn, Are you talking about entries in the (board-specific) mapping table, which I agree would contain the useful subset of combinations of options, or a list of possible settings exposed by the SoC driver, which would have to expose all possibilities, or they wouldn't be available for the mapping table to select from? In the case of a list of possible settings exposed by the SoC driver, * If such a list (of combinations) were to exist, I think it'd need to include all combinations (the cross-product of all individual config params) in general. * I can certainly imagine that for some SoCs, or for a particular device on a SoC, or for a particular board, only a subset of those would be useful, and hence a limited set would be useful. However, that selection is up to the board mapping table not the SoC driver in general. * In Tegra's case at least, I think a number of the numeric values (e.g. pull strength with range 0..31) may be for board calibration, and besides that, most combinations of param values would be useful in some case, and hence we'd always have to expose everything, in order to allow the board mapping table to be able to pick anything the designer needed. * As such, I think the SoC driver should at most list the legal range for each param individually, and let the board-specific mapping table choose the combination(s) required. Yes, I meant the list of settings exposed by pinctrl driver. But it seems that in your case you need to list all the possible combinations of pin configurations. Then I agree it's impractical to have pinctrl driver to maintain this list. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: pinctrl_config APIs, and other pinmux questions
It might be a good place for me to catch up the pinctrl subsystem discussion, as far as imx migration concerned. I have not read the backlog of all the previous discussion, so please excuse me if something I put here have been discussed. On Thu, Oct 13, 2011 at 01:59:55PM -0700, Stephen Warren wrote: Linus, (and other people interested in pinmux!) I've started to look at porting the Tegra pinmux driver to your pinctrl subsystem. In order to replace the existing pinmux driver, I need a way to support configuring options such as tri-state, pull-up/down, drive- strength, etc. I'm also concerned about this. The current pinctrl subsystem only implements pinmux. Before pin/pad configuration (pincfg) support is available, I can not really convert imx iomuxc driver (iomux-v3) over to use the subsystem. At this point, I have a couple of options: 1) Initialize all those parameters (and indeed the initial system-level mux configuration) using custom code in the Tegra pinmux driver, and just use the pin controller subsystem APIs for dynamic pin-muxing. As you recall, I did post some patches to the Tegra pinmux driver to do this, but I'm hoping to do (2) instead. 2) Enhance the pin controller subsystem to support configuring these properties. Yeah. I remember Linus.W originally named the subsystem pinmux and changed it to pinctrl later for that he wants to cover not only pinmux but also pin/pad configuration with this subsystem. The following is some issues that need to be thought about if we do (2): Issue 1: Pinctrl core - driver interface This is something I'm not completely follow right now. Looking at the two drivers already migrated to the subsystem, pinmux-u300 and pinmux-sirf, from the naming of the driver, it seems they are supposed to be pinmux drivers and only handle pinmux. Is it a indication that we will have another pincfg driver to handle pin configuration? In that case, we need to have a clear separation among pinmux, and pincfg and pinctrl (core), e.g. pinctrl_ops probably should not stay in pinmux driver. Alternatively, we can have driver pinctrl-u300 rather than pinmux-u300 handling both pinmux and pincfg in one driver. If pinmux and pincfg are not so standalone, e.g. we may want to expand the mapping to be function-pinmux-pincfg, this option might be good. I propose adding the following to pinctrl_ops: int (*pin_config)(unsigned pin, u32 param, u32 data); int (*group_config)(unsigned group, u32 param, u32 data); If we take pinmux and pincfg as two equal but separate pieces of pinctrl, having pinmux_ops for pinmux while plugging pincfg ops into pinctrl_ops is something looks odd to me. The reasonable way to add pincfg functions into pinctrl_ops would be that we have driver named pincrl-u300 rather than pinmux-u300, both pinmux_ops and pincfg_ops embedded in pinctrl_ops, to handle pinmux and pincfg in the same driver. Where param is some driver-specific parameter, such as: #define TEGRA_PINCTRL_PARAM_TRISTATE_ENABLE0 #define TEGRA_PINCTRL_PARAM_PULL_TYPE 1 #define TEGRA_PINCTRL_PARAM_SCHMITT_ENABLE 2 #define TEGRA_PINCTRL_PARAM_HIGH_SPEED_ENABLE 3 #define TEGRA_PINCTRL_PARAM_DRIVE_STRENGTH 4 #define TEGRA_PINCTRL_PARAM_PULL_UP_STRENGTH 5 #define TEGRA_PINCTRL_PARAM_PULL_DOWN_STRENGTH 6 #define TEGRA_PINCTRL_PARAM_SLEW_RATE_RISING 7 #define TEGRA_PINCTRL_PARAM_SLEW_RATE_FALLING 8 Rationale: Param being SoC-specific: +1 I looked at a bunch of existing pinmux drivers in the mainline kernel. There are certainly some common concepts between SoCs. However, the details are often different enough that I don't think attempting to unify the set of configuration options will be that successful; I doubt we could create a generic enough abstraction of these settings for a cross-SoC driver to be able to usefully name and select values for all the different pin/group options. Data being a plain u32 rather than a pointer as your v7 patchset had it: +1 Using a pointer does allow passing arbitrary data into the driver, which seems useful. However, this also makes it much more difficult for the core pin controller API to handle the values, e.g. in the mapping table, in a device-tree representation thereof, etc. Having both config_pin and config_group functions: Tegra at least has settings that are applicable to groups. Most (all??) other SoCs apply configurations to individual pins. I think we need separate functions to make this explicit, so the function knows if it's looking up selector as a pin name (ID) or a group name (ID). The group defined in this subsystem does not necessarily require multiple pins be controlled by single register at hardware level. A group of pins muxed for given function can be controlled by hardware in the way of individual pin. Similar to pinmux, I guess that the interface between pincfg and core can take group only, and leave the mapping between group and pins
Re: pinctrl_config APIs, and other pinmux questions
On Fri, Oct 14, 2011 at 08:53:33AM -0700, Stephen Warren wrote:
Shawn Guo wrote at Friday, October 14, 2011 8:59 AM:
It might be a good place for me to catch up the pinctrl subsystem
discussion, as far as imx migration concerned.
I have not read the backlog of all the previous discussion, so please
excuse me if something I put here have been discussed.
On Thu, Oct 13, 2011 at 01:59:55PM -0700, Stephen Warren wrote:
Linus, (and other people interested in pinmux!)
I've started to look at porting the Tegra pinmux driver to your pinctrl
subsystem. In order to replace the existing pinmux driver, I need a way
to support configuring options such as tri-state, pull-up/down, drive-
strength, etc.
...
2) Enhance the pin controller subsystem to support configuring these
properties.
Yeah. I remember Linus.W originally named the subsystem pinmux and
changed it to pinctrl later for that he wants to cover not only pinmux
but also pin/pad configuration with this subsystem.
AIUI, the pin control subsystem is intended to encompass both pin muxing
and pin configuration, it's just that the pin configuration part isn't
fleshed out yet, and will be via a separate ops structure that the overall
driver can provide.
...
I propose adding the following to pinctrl_ops:
int (*pin_config)(unsigned pin, u32 param, u32 data);
int (*group_config)(unsigned group, u32 param, u32 data);
...
Having both config_pin and config_group functions:
Tegra at least has settings that are applicable to groups. Most (all??)
other SoCs apply configurations to individual pins. I think we need
separate functions to make this explicit, so the function knows if it's
looking up selector as a pin name (ID) or a group name (ID).
The group defined in this subsystem does not necessarily require
multiple pins be controlled by single register at hardware level.
A group of pins muxed for given function can be controlled by hardware
in the way of individual pin. Similar to pinmux, I guess that the
interface between pincfg and core can take group only, and leave the
mapping between group and pins to the driver. Single-pin function can
be a particular case of group-pins.
That's true; we could define a group for each pin, which no function uses
as a legal group/position, but does allow config() to be used.
Issue 2: Do we need a new pin settings table passed from the board to
the pin controller core?
Issue 3: Do we need to change pin settings on-the-fly, or only at boot?
...
However, if we ever want things to change dynamically (say, suspend/
resume), or only be activated when a device initializes (just like the
mux settings), we need to enhance struct pinmux_map to contain either
mux settings or config settings; perhaps rework it as follows:
I would think that we need to enhance pinmux_map to contain both mux
*and* config settings. To me, the mapping seems to be a group of pins
with specific mux and config settings for a given function.
enum pinmux_map_type {
MUX,
PIN_CONFIG,
GROUP_CONFIG
};
struct pinmux_map {
const char *name;
struct device *ctrl_dev;
const char *ctrl_dev_name;
enum pinmux_map_type type;
union {
struct {
const char *function;
const char *group;
} mux;
struct {
u32 param;
u32 data;
} config;
} data;
struct device *dev;
const char *dev_name;
const bool hog_on_boot;
};
If the config has limited number of possible settings for given
function, we can probably enumerate it in pinctrl driver just like
what we are doing with pinmux option, and add parameter config to
pinmux_map to select pincfg option for given mapping.
@@ -46,6 +46,7 @@ struct pinmux_map {
const char *ctrl_dev_name;
const char *function;
const char *group;
+ const char *config;
struct device *dev;
const char *dev_name;
const bool hog_on_boot;
Such that pinmux_enable would both activate all the mux settings, and
also call pin/group_config based on the mapping table.
Having the driver expose a list of all possible combinations of pin
configurations seems impractical, for the same reason as I objected to
the original proposal for how the driver listed functions; there are
simply far too many pin config parameters and legal value to list the
entire set of combinations.
I did not mean to list the entire set of combinations. For given
function, the applicable number of config should be very limited.
For most cases, it could be just one. For imx6q usdhc example, it's
3, for 50M, 100M and 200M SD bus clock cases.
Regards,
Shawn
Re: [PATCH 1/2] drivers: create a pin control subsystem v9
On Mon, Oct 10, 2011 at 10:23:53AM +0200, Linus Walleij wrote:
On Sun, Oct 9, 2011 at 11:36 AM, Shawn Guo shawn@freescale.com wrote:
+ * @hog_on_boot: if this is set to true, the regulator subsystem will
itself
^
s/regulator/pinmux?
Yep!
+#endif /* !CONFIG_PINCTRL */
s/!CONFIG_PINCTRL/CONFIG_PINMUX?
Yep!
Foldes into original patch with a fixup comment.
Can I have your Reviewed-by: tag on this subsystem?
Sorry, not yet. This is not a full review. I'm trying to migrate imx
pinctrl to the subsystem. And all these are just some random spotting.
Another couple of typo on pinctrl.txt?
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
new file mode 100644
index 000..2915fea
--- /dev/null
+++ b/Documentation/pinctrl.txt
@@ -0,0 +1,951 @@
[...]
+The example 8x8 PGA package above will have pin numbers 0 thru 63 assigned to
+its physical pins. It will name the pins { A1, A2, A3 ... H6, H7, H8 } using
+pinctrl_register_pins_[sparse|dense]() and a suitable data set as shown
It should just be pinctrl_register_pins()?
+earlier.
[...]
+System pinmux hogging
+=
+
+A system pinmux map entry, i.e. a pinmux setting that does not have a device
+associated with it, can be hogged by the core when the pin controller is
+registered. This means that the core will attempt to call regulator_get() and
+regulator_enable() on it immediately after the pin control device has been
+registered.
s/regulator_get/pinmux_get, and s/regulator_enable/pinmux_enable
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/2] drivers: create a pin control subsystem v9
On Mon, Oct 03, 2011 at 10:17:42AM +0200, Linus Walleij wrote:
[...]
diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
new file mode 100644
index 000..2cd4033
--- /dev/null
+++ b/include/linux/pinctrl/machine.h
@@ -0,0 +1,107 @@
+/*
+ * Machine interface for the pinctrl subsystem.
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * Based on bits of regulator core, gpio core and clk core
+ *
+ * Author: Linus Walleij linus.wall...@linaro.org
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef __LINUX_PINMUX_MACHINE_H
+#define __LINUX_PINMUX_MACHINE_H
+
+/**
+ * struct pinmux_map - boards/machines shall provide this map for devices
+ * @name: the name of this specific map entry for the particular machine.
+ * This is the second parameter passed to pinmux_get() when you want
+ * to have several mappings to the same device
+ * @ctrl_dev: the pin control device to be used by this mapping, may be NULL
+ * if you provide .ctrl_dev_name instead (this is more common)
+ * @ctrl_dev_name: the name of the device controlling this specific mapping,
+ * the name must be the same as in your struct device*, may be NULL if
+ * you provide .ctrl_dev instead
+ * @function: a function in the driver to use for this mapping, the driver
+ * will lookup the function referenced by this ID on the specified
+ * pin control device
+ * @group: sometimes a function can map to different pin groups, so this
+ * selects a certain specific pin group to activate for the function, if
+ * left as NULL, the first applicable group will be used
+ * @dev: the device using this specific mapping, may be NULL if you provide
+ * .dev_name instead (this is more common)
+ * @dev_name: the name of the device using this specific mapping, the name
+ * must be the same as in your struct device*, may be NULL if you
+ * provide .dev instead
+ * @hog_on_boot: if this is set to true, the regulator subsystem will itself
^
s/regulator/pinmux?
+ * hog the mappings as the pinmux device drivers are attched, so this is
+ * typically used with system maps (mux mappings without an assigned
+ * device) that you want to get hogged and enabled by default as soon as
+ * a pinmux device supporting it is registered. These maps will not be
+ * disabled and put until the system shuts down.
+ */
+struct pinmux_map {
+ const char *name;
+ struct device *ctrl_dev;
+ const char *ctrl_dev_name;
+ const char *function;
+ const char *group;
+ struct device *dev;
+ const char *dev_name;
+ const bool hog_on_boot;
+};
+
+/*
+ * Convenience macro to set a simple map from a certain pin controller and a
+ * certain function to a named device
+ */
+#define PINMUX_MAP(a, b, c, d) \
+ { .name = a, .ctrl_dev_name = b, .function = c, .dev_name = d }
+
+/*
+ * Convenience macro to map a system function onto a certain pinctrl device.
+ * System functions are not assigned to a particular device.
+ */
+#define PINMUX_MAP_SYS(a, b, c) \
+ { .name = a, .ctrl_dev_name = b, .function = c }
+
+/*
+ * Convenience macro to map a function onto the primary device pinctrl device
+ * this is especially helpful on systems that have only one pin controller
+ * or need to set up a lot of mappings on the primary controller.
+ */
+#define PINMUX_MAP_PRIMARY(a, b, c) \
+ { .name = a, .ctrl_dev_name = pinctrl.0, .function = b, \
+ .dev_name = c }
+
+/*
+ * Convenience macro to map a system function onto the primary pinctrl
device.
+ * System functions are not assigned to a particular device.
+ */
+#define PINMUX_MAP_PRIMARY_SYS(a, b) \
+ { .name = a, .ctrl_dev_name = pinctrl.0, .function = b }
+
+/*
+ * Convenience macro to map a system function onto the primary pinctrl
device,
+ * to be hogged by the pinmux core until the system shuts down.
+ */
+#define PINMUX_MAP_PRIMARY_SYS_HOG(a, b) \
+ { .name = a, .ctrl_dev_name = pinctrl.0, .function = b, \
+ .hog_on_boot = true }
+
+
+#ifdef CONFIG_PINMUX
+
+extern int pinmux_register_mappings(struct pinmux_map const *map,
+ unsigned num_maps);
+
+#else
+
+static inline int pinmux_register_mappings(struct pinmux_map const *map,
+unsigned num_maps)
+{
+ return 0;
+}
+
+#endif /* !CONFIG_PINCTRL */
s/!CONFIG_PINCTRL/CONFIG_PINMUX?
+#endif
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 01/11] MFD: DA9052 MFD core module v2
On Tue, Jul 05, 2011 at 08:07:00PM +0530, ashishj3 wrote:
The DA9052 is a highly integrated PMIC subsystem with supply domain
flexibility
to support wide range of high performance application.
It provides voltage regulators, GPIO controller, Touch Screen, RTC, Battery
control and other functionality.
Signed-off-by: David Dajun Chen dc...@diasemi.com
Signed-off-by: Ashish Jangam ashish.jan...@kpitcummins.com
---
Changes since v2:
- Drop da9052_irqs[] table.
- Move struct da9052_subdev_info[].
- Remove initialization of static member.
- Care for NULL pdata init().
- Check removal of subdevices on errors.
- Remove open source spi code.
- Remove '_spi' from the driver name.
- Move tbat_lookup table from header file.
- Remove irq.h
- Remove num_gpio variable from pdata
---
drivers/mfd/Kconfig | 25 ++
drivers/mfd/Makefile |7 +
drivers/mfd/da9052-core.c | 539 +
drivers/mfd/da9052-i2c.c | 168
drivers/mfd/da9052-irq.c | 168
drivers/mfd/da9052-spi.c | 132 +++
include/linux/mfd/da9052/da9052.h | 91 +
include/linux/mfd/da9052/pdata.h | 43 ++
include/linux/mfd/da9052/reg.h| 777
+
9 files changed, 1950 insertions(+), 0 deletions(-)
create mode 100755 drivers/mfd/da9052-core.c
create mode 100755 drivers/mfd/da9052-i2c.c
create mode 100755 drivers/mfd/da9052-irq.c
create mode 100755 drivers/mfd/da9052-spi.c
create mode 100755 include/linux/mfd/da9052/da9052.h
create mode 100755 include/linux/mfd/da9052/pdata.h
create mode 100755 include/linux/mfd/da9052/reg.h
[...]
+int da9052_reg_update(struct da9052 *da9052, unsigned char reg,
+unsigned char bit_mask, unsigned char reg_val)
+{
+ int ret;
+ unsigned char val;
+
+ if (reg DA9052_MAX_REG_CNT) {
+ dev_err(da9052-dev, invalid reg %x\n, reg);
+ return -EINVAL;
+ }
+
+ mutex_lock_interruptible(da9052-io_lock);
Compile warning as below.
warning: ignoring return value of ‘mutex_lock_interruptible’,
declared with attribute warn_unused_result
+
+ if (da9052-read_dev == NULL || da9052-write_dev == NULL) {
+ ret = -ENODEV;
+ goto err;
+ }
+
+ ret = da9052-read_dev(da9052, reg, 1, val);
+ if (ret 0)
+ goto err;
+
+ val = ~bit_mask;
+ val |= reg_val;
+
+ ret = da9052-write_dev(da9052, reg, 1, val);
+ if (ret 0)
+ goto err;
+
+ mutex_unlock(da9052-io_lock);
+
+ return 0;
+
+err:
+ mutex_unlock(da9052-io_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(da9052_reg_update);
+
+int da9052_set_bits(struct da9052 *da9052, unsigned char reg,
+ unsigned char bit_mask)
+{
+ unsigned char val;
+ int ret;
+
+ if (reg DA9052_MAX_REG_CNT) {
+ dev_err(da9052-dev, invalid reg %x\n, reg);
+ return -EINVAL;
+ }
+
+ mutex_lock_interruptible(da9052-io_lock);
ditto
+
+ if (da9052-read_dev == NULL || da9052-write_dev == NULL) {
+ ret = -ENODEV;
+ goto err;
+ }
+
+ ret = da9052-read_dev(da9052, reg, 1, val);
+ if (ret 0)
+ goto err;
+
+ val |= bit_mask;
+
+ ret = da9052-write_dev(da9052, reg, 1, val);
+ if (ret 0)
+ goto err;
+
+ mutex_unlock(da9052-io_lock);
+
+ return 0;
+
+err:
+ mutex_unlock(da9052-io_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(da9052_set_bits);
+
+int da9052_clear_bits(struct da9052 *da9052, unsigned char reg,
+unsigned char bit_mask)
+{
+ unsigned char val;
+ int ret;
+
+ if (reg DA9052_MAX_REG_CNT) {
+ dev_err(da9052-dev, invalid reg %x\n, reg);
+ return -EINVAL;
+ }
+
+ mutex_lock_interruptible(da9052-io_lock);
ditto
+
+ if (da9052-read_dev == NULL || da9052-write_dev == NULL) {
+ ret = -ENODEV;
+ goto err;
+ }
+
+ ret = da9052-read_dev(da9052, reg, 1, val);
+ if (ret 0)
+ goto err;
+
+ val = ~bit_mask;
+
+ ret = da9052-write_dev(da9052, reg, 1, val);
+ if (ret 0)
+ goto err;
+
+ mutex_unlock(da9052-io_lock);
+
+ return 0;
+
+err:
+ mutex_unlock(da9052-io_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(da9052_clear_bits);
[...]
+static int __devinit da9052_i2c_probe(struct i2c_client *client,
+const struct i2c_device_id *id)
+{
+ struct i2c_adapter *adapter;
+ struct da9052 *da9052_i2c;
+ int ret;
+
+ da9052_i2c = kzalloc(sizeof(struct da9052), GFP_KERNEL);
+ if (!da9052_i2c)
+ return -ENOMEM;
+
+ adapter = to_i2c_adapter(client-dev.parent);
+
+ if (!i2c_check_functionality(adapter,
Re: [PATCH 01/11] MFD: DA9052 MFD core module v2
On Tue, Jul 05, 2011 at 08:07:00PM +0530, ashishj3 wrote:
The DA9052 is a highly integrated PMIC subsystem with supply domain
flexibility
to support wide range of high performance application.
It provides voltage regulators, GPIO controller, Touch Screen, RTC, Battery
control and other functionality.
Signed-off-by: David Dajun Chen dc...@diasemi.com
Signed-off-by: Ashish Jangam ashish.jan...@kpitcummins.com
---
[...]
diff --git a/include/linux/mfd/da9052/pdata.h
b/include/linux/mfd/da9052/pdata.h
new file mode 100755
index 000..75d82a3
--- /dev/null
+++ b/include/linux/mfd/da9052/pdata.h
@@ -0,0 +1,43 @@
+/*
+ * Platform data declarations for DA9052 PMICs.
+ *
+ * Copyright(c) 2011 Dialog Semiconductor Ltd.
+ *
+ * Author: David Dajun Chen dc...@diasemi.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#ifndef __MFD_DA9052_PDATA_H__
+#define __MFD_DA9052_PDATA_H__
+
+#define DA9052_MAX_REGULATORS15
+
+struct da9052;
+
+struct da9052_pdata {
+ struct led_platform_data *pled;
+ struct da9052_wdt_platform_data *pwdt;
+ struct da9052_tsi_platform_data *ptsi;
+ int (*init) (struct da9052 *da9052);
Do you have any actually example to demonstrate the necessity of
this hook? Otherwise, I would suggest we drop it, since machine
code can always do whatever setup needed there.
I'm asking because this kind of hooks always get in the way of the
device tree conversion.
Regards,
Shawn
+ int irq;
+ int irq_base;
+ int num_regulators;
+ int gpio_base;
+ struct regulator_init_data *regulators[DA9052_MAX_REGULATORS];
+};
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: IMX53 loco SATA, how to use it?
On Tue, Jul 19, 2011 at 03:17:00PM +0200, Zygmunt Krynicki wrote: Hello everyone. I was wondering if anyone is using the SATA port on IMX53 loco (aka the quick start). The obvious issue is that of no power to drive the hard disk or SSD. Unless you have an eSATA enclosure and an appropriate SATA - eSATA cable, how would you (or how are you) using this port. Three things come to my mind: 1) Purchase a random ATX power supply unit, connect the 12V rail to the disk, connect the SATA cable to the disk and the board. Short the appropriate cable on the ATX power supply to turn this on. (risky, looks ugly, unsafe). 2) Purchase a USB-SATA dongle, especially the bare-bone model, get a SATA power extender cable, connect one to the USB-SATA board, connect the other to the disk. This way we can power the disk from USB via the adapter and still use the SATA port on IMX. 3) Variant of 2) that could drive 3.25 disks. Purchase a 3.5HDD enclosure, take it apart, assuming it still has cables (not like most 2.5 enclosures that have cable-less connection to the disk) connect just the power cable to the disk and discard the rest. Each solution seems hacky and ugly to me, perhaps one of you has found a better way. I saw Richard Zhu has a setup. So I Cc-ed him for comments. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: IMX53 loco SATA, how to use it?
On Wed, Jul 20, 2011 at 03:01:25PM +0800, Shawn Guo wrote: On Tue, Jul 19, 2011 at 03:17:00PM +0200, Zygmunt Krynicki wrote: Hello everyone. I was wondering if anyone is using the SATA port on IMX53 loco (aka the quick start). The obvious issue is that of no power to drive the hard disk or SSD. Unless you have an eSATA enclosure and an appropriate SATA - eSATA cable, how would you (or how are you) using this port. Three things come to my mind: 1) Purchase a random ATX power supply unit, connect the 12V rail to the disk, connect the SATA cable to the disk and the board. Short the appropriate cable on the ATX power supply to turn this on. (risky, looks ugly, unsafe). 2) Purchase a USB-SATA dongle, especially the bare-bone model, get a SATA power extender cable, connect one to the USB-SATA board, connect the other to the disk. This way we can power the disk from USB via the adapter and still use the SATA port on IMX. 3) Variant of 2) that could drive 3.25 disks. Purchase a 3.5HDD enclosure, take it apart, assuming it still has cables (not like most 2.5 enclosures that have cable-less connection to the disk) connect just the power cable to the disk and discard the rest. Each solution seems hacky and ugly to me, perhaps one of you has found a better way. I saw Richard Zhu has a setup. So I Cc-ed him for comments. Not sure what went wrong. Richard did not get Cc-ed. Another try with both his Freescale and Linaro addresses Cc-ed ... -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH] ARM: mx5: change babbage card_detect and write_protect to use gpio
Due to the issue reported with ESDHC_CD_CONTROLLER mode as below,
GPIO mode becomes the best choice for card detection before the
issue gets addressed.
http://article.gmane.org/gmane.linux.ports.arm.kernel/120790
Signed-off-by: Shawn Guo shawn@linaro.org
---
Actually the issue has been fixed with the v3 of the patch set as
below, but it's still the safest to use gpio mode for card detection
to let the monthly release go independently, since the fixing has
not got enough feedback yet.
http://thread.gmane.org/gmane.linux.ports.arm.kernel/121081
arch/arm/mach-mx5/board-mx51_babbage.c | 14 +-
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c
b/arch/arm/mach-mx5/board-mx51_babbage.c
index 6e6de47..ad693ec 100644
--- a/arch/arm/mach-mx5/board-mx51_babbage.c
+++ b/arch/arm/mach-mx5/board-mx51_babbage.c
@@ -41,6 +41,8 @@
#define BABBAGE_POWER_KEY IMX_GPIO_NR(2, 21)
#define BABBAGE_ECSPI1_CS0 IMX_GPIO_NR(4, 24)
#define BABBAGE_ECSPI1_CS1 IMX_GPIO_NR(4, 25)
+#define BABBAGE_SD1_CD IMX_GPIO_NR(1, 0)
+#define BABBAGE_SD1_WP IMX_GPIO_NR(1, 1)
#define BABBAGE_SD2_CD IMX_GPIO_NR(1, 6)
#define BABBAGE_SD2_WP IMX_GPIO_NR(1, 5)
@@ -141,9 +143,9 @@ static iomux_v3_cfg_t mx51babbage_pads[] = {
MX51_PAD_SD1_DATA1__SD1_DATA1,
MX51_PAD_SD1_DATA2__SD1_DATA2,
MX51_PAD_SD1_DATA3__SD1_DATA3,
- /* CD/WP from controller */
- MX51_PAD_GPIO1_0__SD1_CD,
- MX51_PAD_GPIO1_1__SD1_WP,
+ /* CD/WP gpio */
+ MX51_PAD_GPIO1_0__GPIO1_0,
+ MX51_PAD_GPIO1_1__GPIO1_1,
/* SD 2 */
MX51_PAD_SD2_CMD__SD2_CMD,
@@ -340,8 +342,10 @@ static const struct spi_imx_master mx51_babbage_spi_pdata
__initconst = {
};
static const struct esdhc_platform_data mx51_babbage_sd1_data __initconst = {
- .cd_type = ESDHC_CD_CONTROLLER,
- .wp_type = ESDHC_WP_CONTROLLER,
+ .cd_gpio = BABBAGE_SD1_CD,
+ .wp_gpio = BABBAGE_SD1_WP,
+ .cd_type = ESDHC_CD_GPIO,
+ .wp_type = ESDHC_WP_GPIO,
};
static const struct esdhc_platform_data mx51_babbage_sd2_data __initconst = {
--
1.7.4.1
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
[GIT PULL] fix for bug 754254
Hi Nicolas, Could you pull the fix for [Bug 754254] imx51 randomly truncates serial input at 31 characters? It extends the card CD/WP support for mx5 platforms, and adds the board level configuration for mx51evk to fix bug 754254 on this particular board. Other boards need to add their board level configuration to actually enable the support. The following changes since commit dcd71f6729fbee40d28a99cb645c979c3db7b545 ARM: footbridge: fix clock event support are available in the git repository at: git://git.linaro.org/people/shawnguo/linux-2.6.git fix-754254 Chris Ball (2): mmc: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a platform hook. mmc: Replace SDHCI_QUIRK_NO_MULTIBLOCK with a platform hook. Shawn Guo (9): mmc: sdhci: make sdhci-pltfm device drivers self registered mmc: sdhci: eliminate sdhci_of_host and sdhci_of_data mmc: sdhci: make sdhci-of device drivers self registered mmc: sdhci: merge two sdhci-pltfm.h into one mmc: sdhci: change sdhci-pltfm into a module * mmc: sdhci: fix interrupt storm from card detection * mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared * mmc: sdhci-esdhc-imx: remove WP from flag ESDHC_FLAG_GPIO_FOR_CD_WP * mmc: sdhci-esdhc-imx: extend card_detect and write_protect support for mx5 [ The last four patches marked with '*' are the actual fixing, and the others are prerequisite patches from mmc-next tree. ] arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c |3 +- arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c |3 +- arch/arm/mach-imx/mach-mx25_3ds.c |2 + arch/arm/mach-imx/mach-pcm043.c|2 + arch/arm/mach-mx5/board-mx51_babbage.c | 24 ++- .../plat-mxc/devices/platform-sdhci-esdhc-imx.c| 12 + arch/arm/plat-mxc/include/mach/esdhc.h | 25 ++- drivers/mmc/host/Kconfig | 49 ++-- drivers/mmc/host/Makefile | 18 +- drivers/mmc/host/sdhci-cns3xxx.c | 44 - drivers/mmc/host/sdhci-dove.c | 43 - drivers/mmc/host/sdhci-esdhc-imx.c | 218 - drivers/mmc/host/sdhci-esdhc.h |3 +- drivers/mmc/host/sdhci-of-core.c | 253 drivers/mmc/host/sdhci-of-esdhc.c | 93 ++-- drivers/mmc/host/sdhci-of-hlwd.c | 67 +- drivers/mmc/host/sdhci-of.h| 42 drivers/mmc/host/sdhci-pltfm.c | 216 - drivers/mmc/host/sdhci-pltfm.h | 90 +++- drivers/mmc/host/sdhci-tegra.c | 117 +++--- drivers/mmc/host/sdhci.c | 32 ++- drivers/mmc/host/sdhci.h |2 + include/linux/mmc/sdhci-pltfm.h| 35 --- include/linux/mmc/sdhci.h |8 +- 24 files changed, 763 insertions(+), 638 deletions(-) ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC] (early draft) dt: Linux dt usage model documentation
On Mon, Jun 13, 2011 at 07:32:15AM -0600, Grant Likely wrote:
[...]
+About now is a good time to lay out an example. Here is part of the
+device tree for the NVIDIA Tegra board.
+
+/{
+ compatible = nvidia,harmony, nvidia,tegra250;
+ #address-cells = 1;
+ #size-cells = 1;
+ interrupt-parent = intc;
+
+ chosen { };
+ aliases { };
+
+ memory {
+ device_type = memory;
+ reg = 0x 0x4000;
+ };
+
+ soc {
+ compatible = nvidia,tegra250-soc, simple-bus;
+ #address-cells = 1;
+ #size-cells = 1;
+ ranges;
+
+ intc: interrupt-controller@50041000 {
+ compatible = nvidia,tegra250-gic;
+ interrupt-controller;
+ #interrupt-cells = 1;
+ reg = 0x50041000 0x1000, 0x50040100 0x0100 ;
+ };
+
+ serial@70006300 {
+ compatible = nvidia,tegra250-uart;
+ reg = 0x70006300 0x100;
+ interrupts = 122;
+ };
+
+ i2s-1: i2s@70002800 {
It seem dtc does not compile the label name i2s-1. Instead,
i2s_1 seems good.
+ compatible = nvidia,tegra250-i2s;
+ reg = 0x70002800 0x100;
+ interrupts = 77;
+ codec = wm8903;
+ };
+
+ i2c@7000c000 {
+ compatible = nvidia,tegra250-i2c;
+ #address-cells = 1;
+ #size-cells = 1;
#size-cells should be 0 ...
+ reg = 0x7000c000 0x100;
+ interrupts = 70;
+
+ wm8903: codec@1a {
+ compatible = wlf,wm8903;
+ reg = 0x1a;
... Otherwise, reg needs a size here.
+ interrupts = 347;
+ };
+ };
+ };
+
+ sound {
+ compatible = nvidia,harmony-sound;
+ i2s-controller = i2s-1;
+ i2s-codec = wm8903;
+ };
+};
+
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC] (early draft) dt: Linux dt usage model documentation
Hi Grant, On Mon, Jun 13, 2011 at 07:32:15AM -0600, Grant Likely wrote: [...] +Linux board support code calls of_platform_populate(NULL, NULL, NULL) +to kick of discovery of devices at the root of the tree. The +parameters are all NULL because when starting from the root of the +tree, there is no need to provide a starting node (the first NULL), a +parent struct device (the last NULL), and we're not using a match +table (yet). For a board that only needs to register devices, +.init_machine() can be completely empty except for the +of_platform_populate() call. + +In the Tegra example, this accounts for the /soc and /sound nodes, but +what about the children of the soc node? Shouldn't they be registered +as platform devices too? For Linux DT support, the generic behaviour +is for child devices to be registered by the parent's device driver at +driver .probe() time. So, an i2c bus device driver will register a +i2c_client for each child node, an spi bus driver will register +it's spi_device children, and similarly for other bus_types. +According to that model, a driver could be written that binds to the +soc node and simply registers platform_devices for each of it's +children. The board support code would allocate and register an soc +device, an soc device driver would bind to the soc device, and +register platform_devices for /soc/interrupt-controller, /soc/serial, +/soc/i2s, and /soc/i2c in it's .probe() hook. Easy, right? Although I do not quite understand what the soc device driver is here. Looking at the devicetree/test code, I do not find this driver and its .probe() hook. Instead, of_platform_bus_create will create platform_device for the provided device_node, and also recursively create devices for all the child nodes, no? +it is a lot of mucking about for just registering platform devices. + -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: linaro-uboot: device tree without ramdisk and boot_relocate_fdt cause illegal memory access in kernel
On Fri, May 27, 2011 at 04:52:38PM +0800, Barry Song wrote:
Hi all,
i am using linaro uboot(u-boot-linaro-stable.git). i have let our
prima2 board support device tree with some workaround in uboot. two
problems i have meet:
1. device tree without ramdisk
now uboot used commands like
bootm kernel_address ramdisk_address dtb_address
to start linux kernel.
in many cases, people have no ramdisk at all, but the following codes
will still stop people to use device tree to start kernel since it got
an illegal ramdisk:
common/cmd_bootm.c:
if (((images.os.type == IH_TYPE_KERNEL) ||
(images.os.type == IH_TYPE_MULTI))
(images.os.os == IH_OS_LINUX)) {
/* find ramdisk */
ret = boot_get_ramdisk (argc, argv, images, IH_INITRD_ARCH,
images.rd_start, images.rd_end);
if (ret) {
puts (Ramdisk image is corrupt or invalid\n);
return 1;
}
#if defined(CONFIG_OF_LIBFDT)
/* find flattened device tree */
ret = boot_get_fdt (flag, argc, argv, images,
images.ft_addr, images.ft_len);
if (ret) {
puts (Could not find a valid device tree\n);
return 1;
}
then i delete the first return 1 to let uboot ignore the ramdisk checking.
2. boot_relocate_fdt in common/image.c
this function will relocate fdt to an new address by:
lmb_alloc_base(lmb, of_len, 0x1000, getenv_bootm_mapsize() +
getenv_bootm_low())
but the return address is probably not in the initilized scale which
kernel will build mapping in head.S. then in the function
setup_machine_fdt() of arch/arm/kernel/devtree.c, when executing:
devtree = phys_to_virt(dt_phys);
/* check device tree validity */
if (be32_to_cpu(devtree-magic) != OF_DT_HEADER)
return NULL;
kernel will die due to illegal memory access since dt_phys was not
mapped to virtual address yet.
For problem1 , could uboot have a way to ignore ramdisk by itself?
since we need 3 param in bootm to support device tree. For problem2,
bootm kernel_address - dtb_address
Use '-' for ramdisk address, if you do not have a ramdisk image.
could uboot just relocate fdt to the original address of old ATAG,
OFF+ 0x100?
Do you have the following commit on your kernel tree?
commit 4d901c4271951d110afb13ee9aa73d27a6c8e53d
Author: Rob Herring rob.herr...@calxeda.com
Date: Wed Feb 2 16:33:17 2011 +0100
ARM: 6648/1: map ATAGs when not in first 1MB of RAM
If ATAGs or DTB pointer is not within first 1MB of RAM, then the boot params
will not be mapped early enough, so map the 1MB region that r2 points to.
Only
map the first 1MB when r2 is 0.
Some assembly improvements from Nicolas Pitre.
Acked-by: Tony Lindgren t...@atomide.com
Acked-by: Nicolas Pitre nicolas.pi...@linaro.org
Signed-off-by: Rob Herring rob.herr...@calxeda.com
Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
You can get it from linux-linaro-2.6.38 tree.
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: http git access?
On Wed, May 25, 2011 at 12:59:54PM +0200, Dirk Behme wrote: On Wed, May 25, 2011 at 8:31 AM, Shawn Guo shawn@freescale.com wrote: On Tue, May 24, 2011 at 06:40:28PM -0400, James Westby wrote: On Sat, 14 May 2011 23:45:35 +0200, Jeremiah Foster jeremiah.fos...@pelagicore.com wrote: It certainly is technically possible. The git-http-backend file allows one to use http and https as transports. Whoever is the admin for the Linaro git repos would need to make some changes to the apache config files for the git repo. See http://www.kernel.org/pub/software/scm/git/docs/git-http-backend.html and http://progit.org/2010/03/04/smart-http.html Hi, Thanks to Eric Miao and Paul Collins this is now enabled on git.linaro.org and gitweb should now be publishing the http urls too. Many thanks for this! Would someone please test and let me know the outcome of that test? Seems not working yet for me: $ git clone http://git.linaro.org/kernel/linux-linaro-2.6.38.git Cloning into linux-linaro-2.6.38... fatal: http://git.linaro.org/kernel/linux-linaro-2.6.38.git/info/refs not found: did you run git update-server-info on the server? Could you check your URL? It seems to me that a 'git' is missing? ..org/git/kernel/...? I tried it and got temp$ git clone http://git.linaro.org/git/kernel/linux-linaro-2.6.38.git Ah, yes. I missed 'git' in the url. As the url for git protocol is git://git.linaro.org/kernel/linux-linaro-2.6.38.git, I thought all I need to do is changing git:// to http://;. Thanks for pointing this out. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Boot sanity testing of release candidate kernel
On Tue, May 24, 2011 at 07:42:43PM -0700, Deepak Saxena wrote: Hi all, The Kernel Working Group is getting ready to release the first of our new monthly development snapshot in a few days and we would like folks to do some quick sanity boot testing on their boards. Please grab or update the kernel from git://git.linaro.org/kernel/linux-linaro-2.6.38.git, and checkout the linaro-11.05-2.6.38 tag (which happens to be same as master at this moment) and give it a quick spin. Note that this is just the stock Linaro kernel and does not include any binary graphics drivers or other bits provided by Linaro's landing team kernels, so we just need the basic build and boot tested along with some simple testing of devices that can be used w/o extra drivers. Boot-tested on mx51evk (babbage). -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: where is the powertop git tree
On Thu, May 26, 2011 at 11:37:17AM +0800, Jello huang wrote: Dear all, i need to test for the arm platform,but I do not find the git tree of powertop on launchpad.net,or the tree is located on linaro.org? http://git.linaro.org/gitweb?p=tools/powertop.git;a=summary -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
LTP test result collecting
I'm drafting the blueprint [1], and need your favors to collect LTP (Linux Test Project [2]) result on boards that Linaro supports with Natty kernel running on. The example steps of the testing (I did on i.mx51 babbage with Beta-2 linaro-n-developer) are documented on Whiteboard of [1]. Please help run the test on the boards you have except i.mx51, and send me the result. I appreciate the effort. [1] https://blueprints.launchpad.net/linux-linaro/+spec/linaro-kernel-o-ras-fix-ltp [2] http://ltp.sourceforge.net/ -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LTP test result collecting
On Mon, May 02, 2011 at 11:00:48AM +0200, Zygmunt Krynicki wrote: W dniu 02.05.2011 10:59, Shawn Guo pisze: I'm drafting the blueprint [1], and need your favors to collect LTP (Linux Test Project [2]) result on boards that Linaro supports with Natty kernel running on. Have you seen abrek, launch-control and lava? You can run LTP via abrek on any board, submit results to launch-control, automate the whole process with lava and poke at the results to create reports with launch-control dashboard. Great. That's the info I'm looking for. Since Validation team has LTP in Linaro test pool, I would utilize this great infrastructural support to make my lift easier. With the info given by Paul Larson, I have got abrek ltp running on my setup, and will try launch-control/lava out later for collecting results on other boards. Thanks, both. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Help on general file sharing
I have one 35MB tarball to share with Linaro folks. It's too big to distribute through email. Is there any infrastructural solution for such general file sharing purpose? (The launchpad PPA is for package than general file sharing, and I do not want to bother.) Thanks. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Help on general file sharing
On Fri, Apr 29, 2011 at 10:10:57AM +0200, Arnd Bergmann wrote: On Friday 29 April 2011 09:23:23 Shawn Guo wrote: I have one 35MB tarball to share with Linaro folks. It's too big to distribute through email. Is there any infrastructural solution for such general file sharing purpose? (The launchpad PPA is for package than general file sharing, and I do not want to bother.) Thanks. What are the contents? If it's source code, I'd suggest you unpack it and put it on git.linaro.org. Then you can make incremental updates in case you have new versions. It's a test suite in binary. And I need favors from others to collect results on those boards that I do not have. I'm going for the launchpad project file download way suggested by Martin. Thanks, all. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: Add dtbuImage target
On Sat, Apr 23, 2011 at 10:12:02PM -0600, Stephen Warren wrote: U-Boot wrapped dtbImage; useful for testing DT with an unmodified U-Boot. Signed-off-by: Stephen Warren swar...@nvidia.com --- This patch is based on: git://kernel.ubuntu.com/jk/dt/linux-2.6.git dtbimage However, I actually developed and tested it only on: git://git.secretlab.ca/git/linux-2.6 devicetree/test plus the following commits from jk's dtbimage branch: 4c2eddb89542f73fe626813e3cdafc789f931aec arm: dtbImage support 4cb80ac96489220554d28f6fde527aeef83e628b arm/dtbimage: copy dtb blob to offset from image base I wonder if rather than simply applying my patch below to jk's dtbimage branch, perhaps it would be better to cherry-pick the two commits I mentioned above into the devicetree/test tree, after which I can submit the original change I wrote there? Speaking of having dtb embedded in zImage, I bet the following patch from John Bonesio gets the better chance to hit the mainline. [PATCHv3] ARM:boot:device tree: Allow the device tree binary to be appended to zImage http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-March/005062.html -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Device Tree hwpacks
On Thu, Apr 21, 2011 at 06:53:50PM +0200, Loïc Minier wrote: Hey If you try latest daily hwpacks + latest linaro-image-tools (0.4.4 or bzr) you should be getting Device Tree aware images for boards which support it (mostly i.MX51 and OMAP ATM). Feedback and bug reports welcome! Just tested on mx51 babbage, and it's working out of box. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 0/5] consolidate sdhci pltfm OF drivers and get them self registered
Hi Wolfram, Thanks for the review. On Tue, Apr 19, 2011 at 12:20:31PM +0200, Wolfram Sang wrote: [...] The approach seems sensible, so have a look at my (mostly minor) comments inside the patches. However, there is one bigger piece missing. You converted all the drivers which had a seperate source-file and hooked into sdhci-pltfm.c. However, those are only those users which need additional code to work around the quirks. There are also users which can take the plain pltfm-driver with a properly set platform_data (check the thread [PATCH] mmc: add SDHCI driver for STM platforms (V2) for an example). Those have to be converted, too. Even those drivers need pltfm-something.c to accommodate the platform_data, right? I think sdhci-dove.c (sitting on mainline) is also such an example. So if I'm not mistaken, I did take care of the drivers which can take the current plain pltfm-sdhci driver. Now the discussion could be if every of those users gets its own pltfm-something.c or if we create something similat to sdhci-pltfm-generic, which can also be setup with platform_data like the old driver (/me likes the latter a bit more. If we don't change the name of the driver (not talking about the sourcefile) and keep it sdhci-pltfm, then you wouldn't need to change all those users if you ensured it behaves the same. Since there are already pltfm-something.c to hold platform_data for those users anyway, it's not an argument here. Also, I think the next version of this series should have all makers of a sdhci-pltfm user CCed so we give them a chance to report breakage. Or donate acks or tested-by. Ok, will do. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/5] mmc: sdhci: make sdhci-pltfm device drivers self registered
On Tue, Apr 19, 2011 at 12:20:42PM +0200, Wolfram Sang wrote:
On Fri, Mar 25, 2011 at 04:48:47PM +0800, Shawn Guo wrote:
The patch turns the common stuff in sdhci-pltfm.c into functions, and
add device drivers their own .probe and .remove which in turn call
into the common functions, so that those sdhci-pltfm device drivers
register itself and keep all device specific things away from common
sdhci-pltfm file.
Signed-off-by: Shawn Guo shawn@linaro.org
---
I'll second the comments from Grant (with one slight exception which is
noted below)
+static int __devexit sdhci_dove_remove(struct platform_device *pdev)
+{
+ struct sdhci_host *host = platform_get_drvdata(pdev);
+ int dead = 0;
+ u32 scratch;
+
+ scratch = readl(host-ioaddr + SDHCI_INT_STATUS);
+ if (scratch == (u32)-1)
+ dead = 1;
I'd prefer
dead = (readl() == 0x);
(or (u32)-1 if you prefer). But keeping a variable 'dead' is more
descriptive than keeping 'scratch'.
Ok.
+MODULE_LICENSE(GPL v2);
Just to be sure: Did you double-check if the original licenses were v2
or v2+?
It seems to me that sdhci-cns3xxx.c, sdhci-dove.c, sdhci-esdhc-imx.c
and sdhci-tegra.c all use v2.
Actually I do not even know how v2+ is stated. Do you have an example
for me to refer?
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
[...]
-err_add_host:
- if (pdata pdata-exit)
- pdata-exit(host);
-err_plat_init:
- iounmap(host-ioaddr);
err_remap:
release_mem_region(iomem-start, resource_size(iomem));
err_request:
sdhci_free_host(host);
err:
- printk(KERN_ERRProbing of sdhci-pltfm failed: %d\n, ret);
- return ret;
+ pr_err(%s failed %d\n, __func__, ret);
dev_err?
Good catch.
+ return NULL;
}
I didn't pay much attention to the OF version of the tegra driver, since
it still is not upstream yet, right?
Do not worry about that. You will not see that in the next version,
which will be based on mmc-next. And tegra OF support should be in
another patch.
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 2/5] mmc: sdhci: eliminate sdhci_of_host and sdhci_of_data
On Tue, Apr 19, 2011 at 12:20:53PM +0200, Wolfram Sang wrote:
On Fri, Mar 25, 2011 at 04:48:48PM +0800, Shawn Guo wrote:
The patch is to migrate the use of sdhci_of_host and sdhci_of_data
to sdhci_pltfm_host and sdhci_pltfm_data, so that the former pair can
be eliminated.
Signed-off-by: Shawn Guo shawn@linaro.org
---
[...]
diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h
index a3e4be0..12afe86 100644
--- a/drivers/mmc/host/sdhci-pltfm.h
+++ b/drivers/mmc/host/sdhci-pltfm.h
@@ -19,6 +19,10 @@
struct sdhci_pltfm_host {
struct clk *clk;
u32 scratchpad; /* to handle quirks across io-accessor calls */
+
+ /* migrate from sdhci_of_host */
+ unsigned int clock;
+ u16 xfer_mode_shadow;
xfer_mode_shadow can be merged into scratchpad. They both fix the same
issue.
Yeah.
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 3/5] mmc: sdhci: make sdhci-of device drivers self registered
On Tue, Apr 19, 2011 at 12:21:01PM +0200, Wolfram Sang wrote:
+static int __devinit sdhci_esdhc_probe(struct platform_device *pdev)
+{
+ struct sdhci_host *host;
+ int ret;
+
+ host = sdhci_pltfm_init(pdev, sdhci_esdhc_pdata);
+ if (!host)
+ return -ENOMEM;
Just noticed: Since pltfm_init may fail due to various reasons, maybe
ERRPTR might be a good idea?
Ok.
[...]
+static int __init sdhci_hlwd_init(void)
+{
+ return platform_driver_register(sdhci_hlwd_driver);
+}
+module_init(sdhci_hlwd_init);
+
+static void __exit sdhci_hlwd_exit(void)
+{
+ platform_driver_unregister(sdhci_hlwd_driver);
+}
+module_exit(sdhci_hlwd_exit);
+
+MODULE_DESCRIPTION(Secure Digital Host Controller Interface OF driver);
+MODULE_AUTHOR(Xiaobo Xie x@freescale.com,
+ Anton Vorontsov avoront...@ru.mvista.com);
+MODULE_LICENSE(GPL v2);
Please double check the authors. It is based on the fsl driver, but the
copyright should go to
* Copyright (C) 2009 The GameCube Linux Team
* Copyright (C) 2009 Albert Herranz
I think.
Sorry, I misread the current copyright in sdhci-of-hlwd.c. You are
right.
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 4/5] mmc: sdhci: consolidate sdhci-of-esdhc and sdhci-esdhc-imx
On Tue, Apr 19, 2011 at 12:21:10PM +0200, Wolfram Sang wrote:
config MMC_SDHCI_ESDHC_IMX
- bool SDHCI platform support for the Freescale eSDHC i.MX controller
+ bool SDHCI support for the Freescale eSDHC i.MX controller
depends on ARCH_MX25 || ARCH_MX35 || ARCH_MX5
depends on MMC_SDHCI
- select MMC_SDHCI_PLTFM
+ select MMC_SDHCI_ESDHC
select MMC_SDHCI_IO_ACCESSORS
help
- This selects the Freescale eSDHC controller support on the platform
- bus, found on platforms like mx35/51.
+ This selects the Freescale eSDHC controller support on platforms
+ like mx35/51.
While we are at it, mx25 could be added and mx53 (and you know better
what else will be coming ;))
It's a direct copy. Will update it properly.
diff --git a/drivers/mmc/host/sdhci-esdhc.c b/drivers/mmc/host/sdhci-esdhc.c
new file mode 100644
index 000..b3d1bc1
--- /dev/null
+++ b/drivers/mmc/host/sdhci-esdhc.c
@@ -0,0 +1,413 @@
+/*
+ * Freescale eSDHC controller driver for MPCxxx and i.MX.
+ *
+ * Copyright (c) 2007 Freescale Semiconductor, Inc.
+ * Author: Xiaobo Xie x@freescale.com
+ *
+ * Copyright (c) 2009 MontaVista Software, Inc.
+ * Author: Anton Vorontsov avoront...@ru.mvista.com
+ *
+ * Copyright (c) 2010 Pengutronix e.K.
+ * Author: Wolfram Sang w.s...@pengutronix.de
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ */
+
+#include linux/io.h
+#include linux/delay.h
+#include linux/err.h
+#include linux/clk.h
+#include linux/mmc/host.h
+#include linux/mmc/sdhci-pltfm.h
+#ifdef CONFIG_MMC_SDHCI_ESDHC_IMX
+#include mach/hardware.h
+#endif
+#include sdhci.h
+#include sdhci-pltfm.h
+
+/*
+ * Ops and quirks for the Freescale eSDHC controller.
+ */
+
+#define ESDHC_DEFAULT_QUIRKS (SDHCI_QUIRK_FORCE_BLK_SZ_2048 | \
+ SDHCI_QUIRK_BROKEN_CARD_DETECTION | \
+ SDHCI_QUIRK_NO_BUSY_IRQ | \
+ SDHCI_QUIRK_NONSTANDARD_CLOCK | \
+ SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK | \
+ SDHCI_QUIRK_PIO_NEEDS_DELAY | \
+ SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET | \
+ SDHCI_QUIRK_NO_CARD_NO_RESET)
These are not the current quirks. Meanwhile BROKEN_CARD_DETECTION is
gone as well as NO_CARD_NO_RESET (at least for imx). My additions to use
GPIOs for card detect and write protect are also not in this version.
Next version will be based on mmc-next.
[...]
+static struct sdhci_pltfm_data sdhci_esdhc_mpc_pdata = {
+ .quirks = ESDHC_DEFAULT_QUIRKS,
+ .ops = sdhci_esdhc_mpc_ops,
+};
+#endif
Please mark the #endif with comments of the expression they are
depending on, e.g.
#endif /* CONFIG_MMC_SDHCI_ESDHC_MPC */
if it is not immediately visible. That helps readability.
Ok, since you like it :)
[...]
Phew, due to all these hardware bugs, the driver will get messy, even if
we try hard. Any chances that future revisions of the core will be
updated? :)
I doubt all these quirks are design bugs but some designs not fully
compliant to SDHC specification, which you still can say bugs :)
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 00/12] mmc: use nonblock mmc requests to minimize latency
Hi Per, On Wed, Apr 06, 2011 at 09:07:01PM +0200, Per Forlin wrote: [...] Per Forlin (12): mmc: add none blocking mmc request function mmc: mmc_test: add debugfs file to list all tests mmc: mmc_test: add test for none blocking transfers mmc: add member in mmc queue struct to hold request data mmc: add a block request prepare function mmc: move error code in mmc_block_issue_rw_rq to a separate function. mmc: add a second mmc queue request member mmc: add handling for two parallel block requests in issue_rw_rq mmc: test: add random fault injection in core.c omap_hsmmc: use original sg_len for dma_unmap_sg omap_hsmmc: add support for pre_req and post_req mmci: implement pre_req() and post_req() drivers/mmc/card/block.c | 493 +++-- drivers/mmc/card/mmc_test.c | 342 - drivers/mmc/card/queue.c | 171 +-- drivers/mmc/card/queue.h | 31 ++- drivers/mmc/core/core.c | 132 ++- drivers/mmc/core/debugfs.c|5 + drivers/mmc/host/mmci.c | 146 +++- drivers/mmc/host/mmci.h |8 + drivers/mmc/host/omap_hsmmc.c | 90 +++- include/linux/mmc/core.h |9 +- include/linux/mmc/host.h | 13 +- lib/Kconfig.debug | 11 + 12 files changed, 1172 insertions(+), 279 deletions(-) I'm playing the patch set and seeing the following warnings. CC drivers/mmc/card/block.o drivers/mmc/card/block.c: In function ‘mmc_blk_issue_rq’: drivers/mmc/card/block.c:429:6: warning: ‘status’ may be used uninitialized in this function CC drivers/mmc/core/core.o drivers/mmc/core/core.c: In function ‘mmc_request_done’: drivers/mmc/core/core.c:163:3: warning: passing argument 2 of ‘mmc_should_fail_request’ from incompatible pointer type drivers/mmc/core/core.c:129:20: note: expected ‘struct mmc_data *’ but argument is of type ‘struct mmc_request *’ -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 0/5] consolidate sdhci pltfm OF drivers and get them self registered
Hi Wolfram, On Thu, Mar 31, 2011 at 12:36:53PM -0400, Chris Ball wrote: Hi Wolfram, On Fri, Mar 25 2011, Shawn Guo wrote: Here are what the patch set does. * Remove .probe and .remove hooks from sdhci-pltfm.c and make it be a pure common helper function providers. * Add .probe and .remove hooks for sdhci pltfm drivers sdhci-cns3xxx, sdhci-dove, sdhci-tegra, and sdhci-esdhc-imx to make them self registered with calling helper functions created above. * Migrate the use of sdhci_of_host and sdhci_of_data to sdhci_pltfm_host and sdhci_pltfm_data, so that OF version host and data structure works can be saved, and pltfm version works for both cases. * Add OF common helper stuff into sdhci-pltfm.c, and make OF version sdhci drivers sdhci-of-esdhc and sdhci-of-hlwd become self registered as well, so that sdhci-of-core.c and sdhci-of.h can be removed. * Consolidate the OF and pltfm esdhc drivers into one with sharing the same pair of .probe and .remove hooks. As a result, sdhci-esdhc-imx.c and sdhci-of-esdhc.c go away, while sdhci-esdhc.c comes in and works for both MPCxxx and i.MX. * Eliminate include/linux/mmc/sdhci-pltfm.h with moving stuff into drivers/mmc/host/sdhci-pltfm.h. If you get time, would appreciate getting an ACK from you on the patch changes (including Grant's review). Thanks very much! Can you please take some time to help review? I know you are on ELC this week :) What about next couple of weeks? -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Sound hardware device specifics for Linaro dev boards
On Fri, Apr 01, 2011 at 10:20:49AM -0500, Kurt Taylor wrote: Hi everyone, Hi Kurt, I am trying to assemble a reference for the sound device specifics for all the platforms. The catch is that I don't have all the dev platforms available to me. Here is where you can help! I have Panda and Beagle C4. I need everything else. If you have access to other platform(s), please do the following and email the output to me. I'm not sure that mx51 babbage is one of the platforms you are concerned here, as there is no sound driver for this platform in Linaro kernel tree (.38) yet. (Well, the driver just went to mainline in .39 merge window.) But anyway, I just tried Beta release with ALIP image, and here is my serial output. [...] * Documentation: http://www.linaro.org * Starting bluetooth[ OK ] * PulseAudio configured for per-user sessions root@linaro:~# pacmd list-sinks No PulseAudio daemon running, or not running as session daemon. I guess something went wrong since line * PulseAudio configured ... as I can see my serial console color changing to yellow from black from there. (no [OK] after the line?) -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 4/5] mmc: sdhci: consolidate sdhci-of-esdhc and sdhci-esdhc-imx
On Thu, Mar 31, 2011 at 09:53:12AM -0600, Grant Likely wrote:
On Fri, Mar 25, 2011 at 04:48:50PM +0800, Shawn Guo wrote:
This patch is to consolidate SDHCI driver for Freescale eSDHC
controller found on both MPCxxx and i.MX platforms. It turns
sdhci-of-esdhc.c and sdhci-esdhc-imx.c into one sdhci-esdhc.c,
which gets the same pair of .probe and .remove serving two cases.
Signed-off-by: Shawn Guo shawn@linaro.org
---
drivers/mmc/host/Kconfig | 38 ++--
drivers/mmc/host/Makefile |3 +-
drivers/mmc/host/sdhci-esdhc-imx.c | 210 --
drivers/mmc/host/sdhci-esdhc.c | 413
drivers/mmc/host/sdhci-of-esdhc.c | 162 --
This patch would be easier to review if it was split into two patches;
first rename sdhci-esdhc-imx.c to sdhci-esdhc.c without any changes to
the .c code, and then a second patch to merge the ppc bits into the
imx version.
OK.
+#if defined(CONFIG_OF)
+#include linux/of_device.h
+static const struct of_device_id sdhci_esdhc_dt_ids[] = {
+#ifdef CONFIG_MMC_SDHCI_ESDHC_IMX
+ { .compatible = fsl,imx-esdhc, .data = sdhci_esdhc_imx_pdata },
+#endif
+#ifdef CONFIG_MMC_SDHCI_ESDHC_MPC
+ { .compatible = fsl,mpc8379-esdhc, .data = sdhci_esdhc_mpc_pdata },
+ { .compatible = fsl,mpc8536-esdhc, .data = sdhci_esdhc_mpc_pdata },
+ { .compatible = fsl,esdhc, .data = sdhci_esdhc_mpc_pdata },
+#endif
+ { }
+};
+MODULE_DEVICE_TABLE(platform, sdhci_esdhc_dt_ids);
+
+static const struct of_device_id *
+sdhci_esdhc_get_of_device_id(struct platform_device *pdev)
+{
+ return of_match_device(sdhci_esdhc_dt_ids, pdev-dev);
You can add an empty implementation of of_match_device() to
linux/of_device.h. That would eliminate the need for this function.
Will follow what you suggested later to use .of_match pointer newly
added into struct device.
--
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
