Re: [Call for participation] Bi-Weekly KVM/ARM Technical Sync-up
On Wed, Aug 21, 2013 at 05:09:39PM -0700, Christoffer Dall wrote: Linaro is going to host a bi-weekly sync-up call for technical issues on KVM/ARM development. The KVM 32-bit and 64-bit maintainers as well as the QEMU ARM maintainer will typically be on the call. The first call will be held Tuesday August 27th. I'll point out that I don't do Tuesdays for phone calls (it's one of the days I regularly take as weekend time) so you'll never be able to invite me if you keep this on Tuesdays. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/5] clk: allow reentrant calls into the clk framework
On Mon, Mar 18, 2013 at 01:15:51PM -0700, Mike Turquette wrote: Quoting Ulf Hansson (2013-02-28 01:54:34) On 28 February 2013 05:49, Mike Turquette mturque...@linaro.org wrote: @@ -703,10 +744,29 @@ int clk_enable(struct clk *clk) unsigned long flags; int ret; + /* this call re-enters if it is from the same context */ + if (spin_is_locked(enable_lock) || mutex_is_locked(prepare_lock)) { + if ((void *) atomic_read(context) == get_current()) { + ret = __clk_enable(clk); + goto out; + } + } I beleive the clk_enable|disable code will be racy. What do you think about this scenario: 1. Thread 1, calls clk_prepare - clk is not reentrant - mutex_lock - set_context to thread1. 2. Thread 2, calls clk_enable - above if will mean that get_current returns thread 1 context and then clk_enable continues - spin_lock_irqsave - set_context to thread 2. 3. Thread 1 continues and triggers a reentancy for clk_prepare - clk is not reentrant (since thread 2 has set a new context) - mutex_lock and we will hang forever. Do you think above scenario could happen? I think the solution would be to invent another static atomic_t context; which is used only for fast path functions (clk_enable|disable). That should do the trick I think. Ulf, You are correct. In fact I have a branch that has two separate context pointers, one for mutex-protected functions and one for spinlock-protected functions. Somehow I managed to discard that change before settling on the final version that was published. Err. Do not forget one very important point. Any clock which has clk_enable() called on it must have had clk_prepare() already called _and_ completed. A second clk_prepare() call on the same clock should be a no-op other than to increase the prepare reference count on it. If you do anything else, you are going to get into sticky problems. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On Tue, Mar 12, 2013 at 06:47:53PM -0700, Bill Huang wrote: On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote: On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote: Add the below four notifier events so drivers which are interested in knowing the clock status can act accordingly. This is extremely useful in some of the DVFS (Dynamic Voltage Frequency Scaling) design. PRE_CLK_ENABLE POST_CLK_ENABLE PRE_CLK_DISABLE POST_CLK_DISABLE Signed-off-by: Bill Huang bilhu...@nvidia.com NAK. *Sigh* NO, this is the wrong level to be doing stuff like this. The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and clk_unprepare(). Those two functions are *merely* helpers for drivers who don't wish to make the individual calls. Drivers are still completely free to call the individual functions, at which point your proposal breaks horribly - and they _do_ call the individual functions. I'm proposing to give device driver a choice when it knows that some driver might be interested in knowing its clock's enabled/disabled state change at runtime, this is very important for centralized DVFS core driver. It is not meant to be covering all cases especially for drivers which is not part of the DVFS, so we don't care if it is calling clk_enable/disable directly or not. But you're not giving drivers a choice. You're giving them an ultimatum. Either they use clk_prepare_enable() which must only be called from non- atomic contexts and have the notifiers, or if they need to use the individual functions (which is what they _should_ be doing but people are too lazy to properly convert stuff) they don't get the option of the notifiers at all. This sucks totally, design wise. The whole point of clk_prepare_enable() is that it is a helper function to _only_ do the clk_prepare() call followed by a clk_enable() call and _nothing_ _else_ _what_ _so_ _ever_. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On Tue, Mar 12, 2013 at 10:42:30PM -0600, Stephen Warren wrote: I believe the point Russell is making is not that the idea behind this patch is wrong, but simply that the function where you put the hooks is wrong. The hooks should at least be in clk_enable/clk_disable and not Indeed, remembering that clk_enable/clk_disable can be called from atomic contexts. If the hook needs to be non-atomic (iow, it can schedule) then it can't go into clk_enable/clk_disable, and must go into clk_prepare/clk_unprepare, which is the schedulable half of that API. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On Tue, Mar 12, 2013 at 10:40:04PM -0700, Bill Huang wrote: That will be too bad, it looks like we deadlock in the mechanism, we cannot change existing drivers behavior (that means some call clk_disable/enable directly, some are not), and we cannot hook notifier in clk_disable/enable either, that means there seems no any chance to get what we want, any idea? Look, the whole point is: - Drivers can call clk_enable/clk_disable from their atomic regions to control the clock. Drivers which do this also call clk_prepare/ clk_unprepare from a schedulable context to perform any operations necessary to allow the clock to be used. - Drivers which only ever control the clock from a schedulable context *can* use clk_prepare_enable()/clk_disable_unprepare() to control their clock, which simplifies the coding in the driver. The whole point here is to cater for what is found on different SoCs and not need to keep rewriting the drivers between different SoCs. So, the idea is that: - clk_prepare() does whatever is needed to prepare a clock for use which may require waiting for the clock to be in a state which it can be enabled. In other words, if there is a PLL, the PLL is setup and we wait for it to report that it has locked. - clk_enable() is about turning the clock output on so that the device receives the clock. Now, in the case of a PLL directly feeding a device, it's entirely possible that clk_prepare() ends up providing the clock signal to the device, and clk_enable() does absolutely nothing. Or, if the clock has a gate on it, it's entirely possible that clk_prepare() does nothing, and clk_enable() unmasks the gate to allow the clock to be provided to the device - which can happen from atomic contexts. The whole point about the separation of these two functions is that device driver writers _can_ code their drivers for both situations and not care about how the SoC implements the clocking at all. Why did we end up with this split in the first place? Because we ran into the problem that some SoCs required a sleeping clk_enable() and others didn't, and the whole thing was turning into an incompatible mess. So, please. Realise that clk_prepare() and clk_enable() are the _official_ APIs, and that clk_prepare_enable() is merely a helper function for drivers to allow them to automate the calling of those two functions in succession with _no_ _further_ _processing_ at all. So, if your hooks need to be callable from schedulable contexts, then you need to put them inside clk_prepare(). If your hooks are callable from atomic contexts, then they can go into clk_enable(). But what you can not do is put them into clk_prepare_enable(). ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On Thu, Mar 14, 2013 at 11:22:47PM -0600, Stephen Warren wrote: Is clk_set_rate() only legal to call in non-atomic contexts then? The header file doesn't say, although I guess since many other functions explicitly say they can't, then by omission it can... I think when all this was discussed alongside the clk_prepare/clk_unprepare stuff, that yes, it was decided that clk_set_rate() was non-atomic only. And yes, that is most definitely the case, because the first thing clk_set_rate() does is take a mutex - which will trigger a might_sleep() complaint if ever called from an atomic context. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC v2 1/1] clk: Add notifier support in clk_prepare/clk_unprepare
On Thu, Mar 14, 2013 at 02:31:04AM -0700, Bill Huang wrote: Add the below two notifier events so drivers which are interested in knowing the clock status can act accordingly. This is extremely useful in some of the DVFS (Dynamic Voltage Frequency Scaling) design. CLK_PREPARED CLK_UNPREPARED Signed-off-by: Bill Huang bilhu...@nvidia.com --- drivers/clk/clk.c |3 +++ include/linux/clk.h |2 ++ 2 files changed, 5 insertions(+) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index ed87b24..3292cec 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -550,6 +550,7 @@ void clk_unprepare(struct clk *clk) { mutex_lock(prepare_lock); __clk_unprepare(clk); + __clk_notify(clk, CLK_UNPREPARED, clk-rate, clk-rate); mutex_unlock(prepare_lock); } EXPORT_SYMBOL_GPL(clk_unprepare); @@ -598,6 +599,8 @@ int clk_prepare(struct clk *clk) mutex_lock(prepare_lock); ret = __clk_prepare(clk); + if (!ret) + __clk_notify(clk, CLK_PREPARED, clk-rate, clk-rate); So, on prepare, we notify after we've prepared the clock. On unprepare, we notify after the clock has been shut down. Are you sure that's the correct ordering? Would it not be better to view it in a stack-like fashion, iow: get prepare notify-prepare enable disable notify-unprepare unprepare put ? diff --git a/include/linux/clk.h b/include/linux/clk.h index b3ac22d..16c1d92 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -43,6 +43,8 @@ struct clk; #define PRE_RATE_CHANGE BIT(0) #define POST_RATE_CHANGE BIT(1) #define ABORT_RATE_CHANGEBIT(2) +#define CLK_PREPARED BIT(3) +#define CLK_UNPREPARED BIT(4) This implies that we're only going to have a maximum of 32 reason codes here. Is that enough? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote: Add the below four notifier events so drivers which are interested in knowing the clock status can act accordingly. This is extremely useful in some of the DVFS (Dynamic Voltage Frequency Scaling) design. PRE_CLK_ENABLE POST_CLK_ENABLE PRE_CLK_DISABLE POST_CLK_DISABLE Signed-off-by: Bill Huang bilhu...@nvidia.com NAK. *Sigh* NO, this is the wrong level to be doing stuff like this. The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and clk_unprepare(). Those two functions are *merely* helpers for drivers who don't wish to make the individual calls. Drivers are still completely free to call the individual functions, at which point your proposal breaks horribly - and they _do_ call the individual functions. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v9 2/2] video: drm: exynos: Add pinctrl support to fimd
On Thu, Feb 28, 2013 at 11:03:57PM +0100, Sylwester Nawrocki wrote: Please just use IS_ERR(), let's stop this IS_ERR_OR_NULL() insanity. Yes, indeed. On that topic (and off-topic for this thread, sorry) I've committed a set of patches to remove most users of IS_ERR_OR_NULL() from arch/arm. These are the last remaining ones there - and I don't want to see any more appearing: arch/arm/plat-samsung/clock.c: if (IS_ERR_OR_NULL(clk)) arch/arm/plat-samsung/clock.c: if (!IS_ERR_OR_NULL(clk) clk-ops clk-ops-round_rate) arch/arm/plat-samsung/clock.c: if (IS_ERR_OR_NULL(clk)) arch/arm/plat-samsung/clock.c: if (IS_ERR_OR_NULL(clk) || IS_ERR_OR_NULL(parent)) arch/arm/mach-imx/devices/platform-ipu-core.c: if (IS_ERR_OR_NULL(imx_ipu_coredev)) arch/arm/mach-imx/devices/platform-ipu-core.c: if (IS_ERR_OR_NULL(imx_ipu_coredev)) arch/arm/kernel/smp_twd.c: * We use IS_ERR_OR_NULL() here, because if the clock stubs arch/arm/kernel/smp_twd.c: if (!IS_ERR_OR_NULL(twd_clk)) They currently all legal uses of it - though I'm sure that the samsung clock uses can be reduced to just IS_ERR(). The IMX use looks valid in that imx_ipu_coredev really can be an error pointer (on failure) or NULL if the platform device hasn't yet been created. The TWD one is explained in the comments in the code (if people had to write explanations for using IS_ERR_OR_NULL(), we'd probably have it used correctly!) ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC/PATCH 2/5] kernel_cpustat: convert to atomic 64-bit accessors
On Thu, Feb 21, 2013 at 10:53:07PM +0100, Frederic Weisbecker wrote: That too should be kcpustat_this_cpu_set(), or kcpustat_this_cpu_add() FWIW. But we probably don't need the overhead of atomic_add() that does a LOCK. atomic_set(var, atomic_read(var) + delta) would be better. All we need You mean atomic64_set() and atomic64_read(). Looking at the generic version in lib/atomic64.c, atomic64_add() is cheaper for 32-bit arches because it doesn't involve taking the lock twice. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [Xen-devel] [PATCH 02/24] xen/arm: hypercalls
On Fri, Jul 27, 2012 at 03:39:31PM +0100, Ian Campbell wrote: On Fri, 2012-07-27 at 15:21 +0100, Russell King - ARM Linux wrote: On Fri, Jul 27, 2012 at 02:02:18PM +0100, Stefano Stabellini wrote: +/** + * hypercall.h + * + * Linux-specific hypervisor handling. + * + * Stefano Stabellini stefano.stabell...@eu.citrix.com, Citrix, 2012 + * + * 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; or, when distributed + * separately from the Linux kernel or incorporated into other + * software packages, subject to the following license: + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this source file (the Software), to deal in the Software without Erm, is that an additional restriction on the GPL which prevents me from shipping this code on a CD and charging for the act of creating the CD and shipping it? That would technically make the above statement incompatible with the GPL. There's an or in there. The non-GPL alternative license is the standard one applied by upstream Xen to the interface headers: http://xenbits.xen.org/hg/xen-unstable.hg/file/tip/xen/include/public/COPYING It's the X11/MIT license IIRC, which the FSF say is GPL compatible. http://www.gnu.org/licenses/license-list.html#X11License The same license is used a few other places in the kernel, e.g. the DRM code. Ok, but be aware that you won't be able to take code from the Linux kernel and place it in a file marked with that license header (because the code authors haven't given permission for it to be placed under any other license other than GPLv2.) ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: EXYNOS: PD: Fix duplicate variable
On Wed, May 09, 2012 at 01:50:28PM +0100, Sangwook Lee wrote: struct generic_pm_domain already has a field for name. Use that field instead of creating another field in struct exynos_pm_domain Argh. No. @@ -99,7 +98,7 @@ static __init int exynos_pm_dt_parse_domains(void) if (of_get_property(np, samsung,exynos4210-pd-off, NULL)) pd-is_off = true; - pd-name = np-name; + pd-pd.name = (char *)np-name; Why this cast? Why can't pd-pd.name be correctly typed in the first place? I assume this is because np-name is const. Never EVER cast away const. It's wrong to do so. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Making ARM multiplatform kernels DT-only?
On Thu, May 03, 2012 at 11:31:13PM -0700, Deepak Saxena wrote: On 3 May 2012 07:04, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Thu, May 03, 2012 at 01:50:35PM +, Arnd Bergmann wrote: Hi everyone, I've been discussing multiplatform kernels with a few people recently, and we will have a lot of discussion sessions about this at Linaro Connect in Hong Kong. One question that came up repeatedly is whether we should support all possible board files for each platform in a multiplatform kernel, or just the ones that are already using DT probing. I would like to get a quick poll of opinions on that and I've tried to put those people on Cc that would be most impacted by this, i.e. the maintainers for platforms that have both DT and non-DT board files at the moment. My feeling is that we should just mandate DT booting for multiplatform kernels, because it significantly reduces the combinatorial space at compile time, avoids a lot of legacy board files that we cannot test anyway, reduces the total kernel size and gives an incentive for people to move forward to DT with their existing boards. The counterargument is that we won't be able to support all the boards we currently do when the user switches on multiplatform, but I think that is acceptable. Note that I would still want to allow users to build platforms separately in order to enable the ATAG style board files, even for platforms that are not multiplatform capable. I'm basing my comments off mach-zynq. How about we take the following steps towards it? 1. create arch/arm/include/mach/ which contains standardized headers for DT based implementations. This must include all headers included by asm/ or linux/ includes. This will also be the only mach/ header directory included for code outside of arch/arm/mach-*. This also acts as the 'default' set of mach/* includes for stuff like timex.h and the empty hardware.h 2. DT based mach-* directories do not have an include directory; their include files must be located in the main include/ heirarchy if shared with other parts of the kernel, otherwise they must be in the mach-* directory. What do we do about all the mach-specific driver related headers that are currently littered all over arch/arm/mach*? Things like mach/mx3fb.h and mach/ohci.h? Such platforms with that kind of stuff haven't fully converted to DT, because these sorts of things are platform dependent yet aren't represented in DT. Arnd (or maybe Nicolas?) suggested something along the lines of scripting something to figure out the constants required for a specific driver and pulling them out of mach/*.h and into that driver as a starting point. But that doesn't solve the problem. Take a USB driver where the register layouts are different. To have it work on two different platforms, you'd need to build the driver twice. Not only that, you'd also need to figure out some way to register only one copy of that. So really, the header file thing is just a sign of larger blocking issues to getting those kinds of drivers working on a multiplatform kernel, and no amount of header munging will sort it out. The fact of that situation is the driver concerned is _not_ multiplatform and shouldn't be built in that situation until it is fixed. 3. Allow build multiple mach-* directories (which we already do... see the samsung stuff.) We still have irqs.h being SoC dependent, and we still haven't taken debug-macros.S far enough along to get rid of that. Then there's also the problem of uncompress.h. The last piece of the puzzle is the common clock stuff. There's also some stuff outside of arch/arm that needs cleanup including USB driver refactoring and some other bits that I can't recall atm. Right now we can build one and only one host controller inteface, even as modules. Not too difficult of a problem to solve and not critical to get the SOCs booting, but needed to be usable on real devices. That's already a problem today, and has nothing to do with this current issue - what I'm saying is the problem can't be made to go away through header file stuff, so this issue is not relevant to this discussion. That's not to say it doesn't need to be resolved, because it does. It's just no reason to argue against what I've said. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Making ARM multiplatform kernels DT-only?
On Thu, May 03, 2012 at 10:38:15PM -0700, Deepak Saxena wrote: I'm of the opinion that we support DT only platforms for multi-platform but this is based on the approach of only caring for multi-platform for newer systems and not worrying too much for legacy HW. You do realise that you're advocating that we forcefully regress stuff that works today. Sorry, that's not going to happen. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Making ARM multiplatform kernels DT-only?
On Fri, May 04, 2012 at 03:20:57PM +0100, Wookey wrote: Debian tries very hard not to support anything in the kernel that upstream don't support in the kernel because otherwise it's way too much work. The current list of supplied arm kernels is: iop32x (ThecusN2100, intel SS4000, GLAN tank) ixp4xx (Linksys NSLU2) kirkwood (*plugs, QNAP NAS, OPenRD) orion5x (QNAP NAS, HP mv2120) versatile mx5 omap because that's a good compromise between coverage and 'building 20-odd images'. I have no idea how much of that lot is going to get DTified, but I'm guessing the older stuff won't be? Well, my understanding is that there's DT patches around for Versatile. OMAP and MX5 are both heading for DT. I'm less certain about the Orion and Kirkwood stuff, but as they're only about 4 years old, I would hope that there was some active movement for these. The iop32x and ixp4xx stuff hasn't seen much in the way of maintenance so its highly likely that these won't be converted to DT unless someone with the hardware decides to step up and do it. So, that means your list should reduce down to five kernels, or three if the Orion/Kirkwood stuff gets converted to DT. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Making ARM multiplatform kernels DT-only?
On Fri, May 04, 2012 at 11:39:30AM -0500, Rob Herring wrote: Many of the headers are simply platform_data structs which may still be needed on DT platforms, but could be moved elsewhere. Those should be in include/linux/platform. Then there's also the problem of uncompress.h. The last piece of the puzzle is the common clock stuff. The smp/hotplug/localtimer related functions are still global. Marc Z has posted patches for this, but I haven't seen recent activity. This and clocks were the 2 main issues I saw trying to build 2 platforms together. highbank and picoxcell could be built together since only highbank has clocks and smp. gpio.h is still required, but empty for most platforms. Those empty gpio.h files are definitely a candidate for going into arch/arm/include/mach/gpio.h, and then all those 12-byte mach/gpio.h can be deleted (13 files). We've not had any progress on the gpio.h issue since I did the last round of cleanup; the next stage was to persuade SoC maintainers to get rid of their optimized versions which aren't compatible with multi-platform kernels. I don't know if folk are expecting me to push that forwards or whether there's someone else working on that aspect of it... So this issue really does need to be progressed too. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Making ARM multiplatform kernels DT-only?
On Fri, May 04, 2012 at 10:03:40PM +0200, Linus Walleij wrote: On Fri, May 4, 2012 at 4:35 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: Well, my understanding is that there's DT patches around for Versatile. Is there? There is some in-tree stuff, but haven't seen any other sign of patches. [...] make it clear that noone ever tested an MMC card on a Versatile booted on real hardware using DT. And I strongly suspect there are more instances like that, it seems AACI, GPIO and I2C and I guess whatever you cannot test on QEMU is just unsupported. Given that we've converted stuff like MMCI to use DT, it _should_ be the case that we can just add the appropriate DT description to the existing Versatile DT - we might need to create some new GPIO devices for things like SYSMCI and get rid of the status function, or we could just use the auxdata stuff in the mean time. Either way, we've solved these problems on other platforms, and the shared code has already been fixed or is being fixed for stuff like Versatile Express. Lets also not forget that the VIC code has already been converted to DT. So I don't think there's a big problem here - I think most of the pieces of the jigsaw are in place through converting other platforms. ___ 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 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. 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. ___ 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 10:40:35AM -0500, Rob Lee wrote: Thanks for the attention on this. From what I've understood, I will send another submission that includes the imx cpuidle patchset and Shawn's device tree late initcall patchset and I'll provide explanation of the two separate patchsets in the cover sheet. What I also want to see _along with_ the addition of the init_late callback is a patch set from the _same_ people fixing up the rest of arch/arm/mach-* to use it - you know, like what I do when I introduce these kinds of things. Otherwise I'm going to NAK the change. We can't have new stuff introduced in this way and the older platforms ignored. Forward progress across the board please. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
On Wed, Mar 21, 2012 at 12:41:41PM -0700, Saravana Kannan wrote: The meaning of clk_enable/disable has been changed and they won't work without calling clk_prepare/unprepare. So, these are definitely new APIs. If it weren't new APIs, then none of the general drivers would need to change. Yes and no. I disagree that the meaning of clk_enable/disable() has changed. It hasn't. What has changed is the preconditions for calling those functions, and necessarily so in the interest of being able to unify the different implementations. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor
On Wed, Feb 29, 2012 at 12:58:26PM +, Dave Martin wrote: On Wed, Feb 29, 2012 at 09:56:02AM +, Ian Campbell wrote: On Wed, 2012-02-29 at 09:34 +, Dave Martin wrote: On Tue, Feb 28, 2012 at 12:28:29PM +, Stefano Stabellini wrote: I don't have a very strong opinion on which register we should use, but I would like to avoid r7 if it is already actively used by gcc. But there is no framepointer for Thumb-2 code (?) Peter Maydell suggested there was: r7 is (used by gcc as) the Thumb frame pointer; I don't know if this makes it worth avoiding in this context. Sounds like it might be a gcc-ism, possibly a non-default option? Anyway, I think r12 will be fine for our purposes so the point is rather moot. Just had a chat with some tools guys -- apparently, when passing register arguments to gcc inline asms there really isn't a guarantee that those variables will be in the expected registers on entry to the inline asm. The best you can do is: register unsigned int foo asm(r7) = value; asm(blah %0 : : r (foo)); and ensure that your assembly checks that %0 _is_ r7 and produces a build error if not. See the __asmeq() macro in asm/system.h to find out how to do that. This feature has been missing from ARM GCC for quite a long time - it's used extensively on x86 GCC, where they have one register class per register, so they can do stuff like: asm(blah %0 : : a (value)); and be guaranteed that %0 will be eax. If you need a specific register, this means that you must set up that register explicitly inside the asm if you want a guarantee that the code will work: asm volatile ( movw r12, %[hvc_num]\n\t ... hvc#0 :: [hvc_num] i (NUMBER) : r12 ); Of course, if you need to set up more than about 5 or 6 registers in this way, the doubled register footprint means that the compiler will have to start spilling stuff to the stack. No it won't - it will barf instead - think about it. If you're clobbering r0 - r5, but need to pass in six values in registers, gcc can't use r0-r5 for that, so it must use the remaining registers. It gets rather unhappy with that, and starts erroring out (iirc 'too many reloads' or some similar error.) I've been there. If you want to do it that way, your only option is to store them to memory and pass the address of the block into the assembly, and reload them there. Which is extremely sucky and inefficient. Practically, the register variable plus asm() does seem to work, and seems to work for virtually all gcc versions out there (there have been the odd buggy version, but those bugs appear to get fixed.) ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor
On Wed, Feb 29, 2012 at 02:44:24PM +, Ian Campbell wrote: If you need a specific register, this means that you must set up that register explicitly inside the asm if you want a guarantee that the code will work: asm volatile ( movw r12, %[hvc_num]\n\t Is gcc (or gas?) smart enough to optimise this away if it turns out that %[hvc_num] == r12? No, and it won't do, because %[hvc_num] is specified in these operands: ... hvc#0 :: [hvc_num] i (NUMBER) : r12 to be an integer, not a register. How are system calls implemented on the userspace side? I confess I don't know what the ARM syscall ABI looks like -- is it all registers or is some of it on the stack? It sounds like the solution ought to be pretty similar though. All registers. We have a few which take a pointer to an in memory array, but those are for some old multiplexed syscalls. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor
On Thu, Mar 01, 2012 at 10:27:02AM +, Dave Martin wrote: So, where there's a compelling reason to inline these things, we can use the existing techniques if we're alert to the risks. But in cases where there isn't a compelling reason, aren't we just inviting fragility unnecessarily? The practical experience from the kernel suggests that there isn't a problem - that's not to say that future versions of gcc won't become a problem, and that the compiler guys may refuse to fix it. I think it's a feature that we should be pressing gcc guys for - it's fairly fundamental to any programming which requires interfaces that require certain args in certain registers, or receive results in certain registers. The options over this are basically: 1. refusing to upgrade to any version of gcc which does not allow registers-in-asm 2. doing the store-to-memory reload-in-asm thing 3. hand-coding veneers for every call to marshall the registers Each of those has its down sides, but I suspect with (1), it may be possible to have enough people applying pressure to the compiler guys that they finally see sense on this matter. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 3/4] arm/dts: OMAP4: Add mmc controller nodes and board data
On Fri, Feb 24, 2012 at 03:56:52PM +0530, Rajendra Nayak wrote: I don't know if we can, but even if we could, we take care of the same bootargs working on two boards (say sdp and panda) *if* on sdp I have my filesystem on eMMC and on panda I have it on external card. What happens if I want to use my filesystem on both boards on external card? This is why the bootargs should be part of the information the boot loader passes to the kernel, and not part of the kernel configuration or DT. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] gic : check if there are pending interrupts
On Fri, Feb 24, 2012 at 02:45:48PM +0100, Daniel Lezcano wrote: The following patch checks if there are pending interrupts on the gic. This function is needed for example for the ux500 cpuidle driver. When the A9 cores and the gic are decoupled from the PRCMU, the idle routine has to check if an interrupt is pending on the gic before entering in retention mode. So what happens if an interrupt becomes pending after you've checked it but before you've gone into retention mode? This basically sounds like one big racy idea to me. Hardware which allows you to go into retention mode with interrupts pending which should prevent it (or at least cause it to re-awake) just sounds like a massive fail to me. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 6/6] mmc: omap_hsmmc: Don't expect MMC1 to always have vmmc supply
On Tue, Feb 21, 2012 at 05:43:54PM +0530, S, Venkatraman wrote: On Tue, Feb 21, 2012 at 3:33 PM, Rajendra Nayak rna...@ti.com wrote: @@ -324,8 +302,8 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host *host) mmc_slot(host).ocr_mask = ocr_value; } else { if (!(mmc_slot(host).ocr_mask ocr_value)) { - pr_err(MMC%d ocrmask %x is not supported\n, - host-id, mmc_slot(host).ocr_mask); + pr_err(MMC ocrmask %x is not supported\n, + mmc_slot(host).ocr_mask); You're dropping the MMC number from these error messages. It would be much better to fix them instead. Use dev_info(mmc_dev(host-mmc), blah rather than pr_err(). Drivers should not be using pr_* unless they really do not have a struct device. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Query: Multiple Mappings to Mem and ARMV6+
On Mon, Feb 20, 2012 at 09:48:56AM -0800, viresh kumar wrote: On Feb 20, 2012 4:31 PM, Catalin Marinas catalin.mari...@arm.com wrote: On 16 February 2012 18:14, viresh kumar viresh.li...@gmail.com wrote: On Thu, Feb 16, 2012 at 9:48 AM, Catalin Marinas catalin.mari...@arm.com wrote: The DMA API implementation on ARM takes care of the cache cleaning and invalidating. I believe that this is the reason why we have cache re-invalidation (we invalidated it in dma_map_*() earlier) in dma_unmap_*() calls for ARMv6+ for DMA_FROM_DEVICE. Am i Correct? Yes. But why isn't keeping only the second one sufficient? Why don't we remove it from dma_map_* routines? Please don't think for a moment that anyone likes the idea of having to walk over the cache twice for every DMA operation. We don't. But I can assure you that it's very very necessary. The first run through the affected cache lines on dma_map_*() is there to get rid of any cache lines for the buffer which may be marked 'dirty'. A dirty cache line can be evicted (written back) to memory at _any_ time. If this occurs while the DMA controller is reading data from a device, the results depend on the order which the particular cache line is written by the DMA controller, and by the cache line eviction. If the cache line eviction happens after the DMA controller has written, the DMA'd data will be overwritten by old stale data. So, we must get rid of these dirty cache lines. We can do that either by cleaning the cache lines or invalidating them. We chose to invalidate them because it makes very little difference. For the case where the DMA controller needs to read from memory, we obviously have to ensure that all data is pushed out of the cache for it to be visible to the DMA controller. For this case, merely cleaning the cache is sufficient. The second run through the affected cache lines is to prevent speculative prefetches occuring while the DMA controller is on operation, which could result in old data (before the DMA controller has written its data) being read by the CPU, resulting again in old stale data. I hope it's now clear why we need to run over the buffer twice. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Query: Multiple Mappings to Mem and ARMV6+
On Thu, Feb 16, 2012 at 05:15:20PM +, Catalin Marinas wrote: On Thu, Feb 16, 2012 at 04:41:02PM +, viresh kumar wrote: Sorry for starting the long old thread again, but i have to start it as i was a bit confused. :( We know that we can't have multiple mappings with different attributes to the same physical memory on ARMv6+ machines due to speculative prefetch. So, we have following kind of mappings in kernel now (please correct me if i am wrong): - Low Mem: Mapped at boot time to - Normal Cacheable - Bufferable - ioremap() - blocked on Low Mem, so that we don't create Device type mapping to same mem - dma_alloc_coherent() and others: - Without DMA_MEM_BUFFERABLE selected - gives strongly ordered mem (i.e. Non cacheable - Non Bufferable) - With DMA_MEM_BUFFERABLE selected - gives Normal - Non cacheable - Bufferable mapping - Maybe some other too... I have a doubt with the last mapping mentioned above. We have two mappings possibly to the same physical memory, with different attributes: One is Cacheable and other one is not. Is this allowed by ARM? Because the patch in which Russell blocked ioremap on Low Mem, he clearly mentioned that these attributes are also important and they should be same. Section A3.5.7 in the latest ARM ARM (revC) clarifies the mismatched There's a new version? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Query: Multiple Mappings to Mem and ARMV6+
On Thu, Feb 16, 2012 at 05:29:28PM +, Catalin Marinas wrote: On Thu, Feb 16, 2012 at 05:22:42PM +, Russell King - ARM Linux wrote: On Thu, Feb 16, 2012 at 05:15:20PM +, Catalin Marinas wrote: On Thu, Feb 16, 2012 at 04:41:02PM +, viresh kumar wrote: Sorry for starting the long old thread again, but i have to start it as i was a bit confused. :( We know that we can't have multiple mappings with different attributes to the same physical memory on ARMv6+ machines due to speculative prefetch. So, we have following kind of mappings in kernel now (please correct me if i am wrong): - Low Mem: Mapped at boot time to - Normal Cacheable - Bufferable - ioremap() - blocked on Low Mem, so that we don't create Device type mapping to same mem - dma_alloc_coherent() and others: - Without DMA_MEM_BUFFERABLE selected - gives strongly ordered mem (i.e. Non cacheable - Non Bufferable) - With DMA_MEM_BUFFERABLE selected - gives Normal - Non cacheable - Bufferable mapping - Maybe some other too... I have a doubt with the last mapping mentioned above. We have two mappings possibly to the same physical memory, with different attributes: One is Cacheable and other one is not. Is this allowed by ARM? Because the patch in which Russell blocked ioremap on Low Mem, he clearly mentioned that these attributes are also important and they should be same. Section A3.5.7 in the latest ARM ARM (revC) clarifies the mismatched There's a new version? Rev C has been available for a while. I thought you got it already: No, if no one tells me when a new version is available, then, unless I waste time regularly polling the website below, I have no way to know. http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0406c/index.html It contains the LPAE and virtualisation extensions. -- Catalin ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] sched: generalize CONFIG_IRQ_TIME_ACCOUNTING for X86 and ARM
On Wed, Feb 08, 2012 at 04:48:34AM -0800, Dmitry Antipov wrote: Generalize CONFIG_IRQ_TIME_ACCOUNTING between X86 and ARM, move noirqtime= option to common debugging code. For a bit of backward compatibility, tsc=noirqtime is preserved, but issues a warning. Suggested-by: Venki Pallipadi ve...@google.com Signed-off-by: Dmitry Antipov dmitry.anti...@linaro.org --- arch/arm/kernel/sched_clock.c |3 +++ arch/x86/Kconfig | 11 --- arch/x86/kernel/tsc.c |7 --- include/linux/sched.h |2 ++ lib/Kconfig.debug | 12 lib/Makefile |2 ++ lib/irqtime.c | 12 7 files changed, 35 insertions(+), 14 deletions(-) create mode 100644 lib/irqtime.c diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c index 5416c7c..56d2a9d 100644 --- a/arch/arm/kernel/sched_clock.c +++ b/arch/arm/kernel/sched_clock.c @@ -162,5 +162,8 @@ void __init sched_clock_postinit(void) if (read_sched_clock == jiffy_sched_clock_read) setup_sched_clock(jiffy_sched_clock_read, 32, HZ); + if (!no_sched_irq_time) + enable_sched_clock_irqtime(); Why are you placing this here? sched_clock is available from the point that it's registered, which should be before the first sched_clock() call. +config IRQ_TIME_ACCOUNTING + bool Fine granularity task level IRQ time accounting + depends on (X86 || (ARM HAVE_SCHED_CLOCK)) Even though it's not bad here, please get out of the habbit of throwing unnecessary parens into the mix. It can make stuff more difficult to read and therefore confirm correctness. (I've spent many a time rewriting if() statements because of paren overuse.) This could have been written: depends on X86 || (ARM HAVE_SCHED_CLOCK) However, ARM will always have HAVE_SCHED_CLOCK after the next merge window, so this can become a much simpler: depends on X86 || ARM Apart from these two points, the rest of the patch looks fine to me but the ultimate decision about its acceptability is up to other people. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] sched: generalize CONFIG_IRQ_TIME_ACCOUNTING for X86 and ARM
On Wed, Feb 08, 2012 at 07:15:26AM -0800, Dmitry Antipov wrote: On 02/08/2012 05:18 AM, Russell King - ARM Linux wrote: Why are you placing this here? sched_clock is available from the point that it's registered, which should be before the first sched_clock() call. This is just because I'm thinking about: if (read_sched_clock == jiffy_sched_clock_read) setup_sched_clock(jiffy_sched_clock_read, 32, HZ); else if (!no_sched_irq_time) enable_sched_clock_irqtime(); I suppose that fine granularity task irq time accounting makes no sense if sched_clock() granularity is poor. Let me put it a different way - is there a reason not to do this in setup_sched_clock() so that it becomes available as soon as sched_clock() has been initialized by a platform? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V6 0/7] add a generic cpufreq driver
On Mon, Jan 09, 2012 at 10:52:48PM +0800, Richard Zhao wrote: On Fri, Jan 06, 2012 at 08:53:37AM +0800, Richard Zhao wrote: On Thu, Jan 05, 2012 at 06:16:54AM +0800, Richard Zhao wrote: hi Russell, May I have your ACK, you merge it? Russell, ping would you have time to look at this patch series? Sorry, kept missing this. Yes, patch 1 is fine, and _that_ patch alone gets: Acked-by: Russell King rmk+ker...@arm.linux.org.uk ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] mx53_loco: add DA9053 PMIC support
On Tue, Jan 17, 2012 at 01:10:53AM +0800, Ying-Chun Liu (PaulLiu) wrote: diff --git a/arch/arm/mach-mx5/board-mx53_loco.c b/arch/arm/mach-mx5/board-mx53_loco.c index fd8b524..61dd8c9 100644 --- a/arch/arm/mach-mx5/board-mx53_loco.c +++ b/arch/arm/mach-mx5/board-mx53_loco.c @@ -23,10 +23,21 @@ #include linux/delay.h #include linux/gpio.h You have this ^^^ include, which includes asm/gpio.h, and in turn mach/gpio.h, but... #include linux/i2c.h +#include linux/module.h +#include linux/platform_device.h +#include linux/irq.h +#include linux/interrupt.h +#include linux/err.h +#include linux/regulator/machine.h +#include linux/regulator/fixed.h +#include linux/mfd/da9052/da9052.h +#include linux/mfd/da9052/pdata.h #include mach/common.h #include mach/hardware.h #include mach/iomux-mx53.h +#include mach/irqs.h +#include mach/gpio.h you include mach/gpio.h here? I don't see why you need mach/irqs.h either. That brings into this another question: you're adding 9 new linux/ includes above. How many of those do you _actually_ need? Eg, I don't see you adding anything from linux/err.h to this file, so why do you need this header? Please don't add include files unnecessarily. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: pl330: fix null pointer dereference in pl330_chan_ctrl()
On Fri, Jan 13, 2012 at 12:36:31PM +, Mans Rullgard wrote: This fixes the thrd-req_running field being accessed before thrd is checked for null. The error was introduced in abb959f. Signed-off-by: Mans Rullgard mans.rullg...@linaro.org I don't know what's happening with the PL330 driver, but there's patches around to remove this file and merge it with the DMA engine driver. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 4/7] cpufreq: add clk-reg cpufreq driver
On Mon, Dec 26, 2011 at 09:44:52PM +0800, Richard Zhao wrote: On Mon, Dec 26, 2011 at 11:10:30AM +, Mark Brown wrote: The *call* is there in the regulator subsystem, it's just that none of the drivers back it up with an actual implementation yet. Which turns out to be a good thing as cpufreq can't currently understand variable latencies and the governors don't deal well with non-trivial latencies anyway. but clk API don't have such calls. and many SoCs only adjust clk frequencies, using one single voltage. That's because it's often not known - especially in the case of PLLs, data sheets don't tend to specify how long it takes for the PLL to relock after a requested change. If it's important that the PLL be locked, there will be a bit to poll (or they'll cause the CPU itself to stall while the PLL is not locked.) So, for these kinds of situations, how do you suggest that the clk API provides this information? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 4/7] cpufreq: add clk-reg cpufreq driver
On Tue, Jan 03, 2012 at 09:25:30PM +0800, Richard Zhao wrote: Hi Russel, On 3 January 2012 17:06, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Mon, Dec 26, 2011 at 09:44:52PM +0800, Richard Zhao wrote: On Mon, Dec 26, 2011 at 11:10:30AM +, Mark Brown wrote: The *call* is there in the regulator subsystem, it's just that none of the drivers back it up with an actual implementation yet. Which turns out to be a good thing as cpufreq can't currently understand variable latencies and the governors don't deal well with non-trivial latencies anyway. but clk API don't have such calls. and many SoCs only adjust clk frequencies, using one single voltage. That's because it's often not known - especially in the case of PLLs, data sheets don't tend to specify how long it takes for the PLL to relock after a requested change. If it's important that the PLL be locked, there will be a bit to poll (or they'll cause the CPU itself to stall while the PLL is not locked.) So, for these kinds of situations, how do you suggest that the clk API provides this information? In latest v6 version, I get clk transition latency from dt property, and get regulator transition latency from regulator API. Could you please help review other arm common changes in v6 version? You didn't get my point: how do you specify a clock transition latency for a clock with a PLL when the data sheets don't tell you what that is, and they instead give you a bit to poll? Do you: (a) make up some number and hope that it's representative (b) not specify any transition latency (c) think about the problem _now_ and define what it means for a clock without a transition latency. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Is Pandaboard cpuhotplug working stably?
On Thu, Dec 22, 2011 at 02:19:23PM +0530, Shilimkar, Santosh wrote: + Peter Z On Wed, Dec 21, 2011 at 3:37 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Wed, Dec 21, 2011 at 05:59:07PM +0800, Barry Song wrote: 2011/12/21 Russell King - ARM Linux li...@arm.linux.org.uk: cpu hotplug is basically totally buggered - the preconditions placed upon the bringup code path are basically impossible to satisfy in any shape or form at the moment. There's the requirement that the secondary CPU is marked online and active before interrupts are enabled for the thread migration stuff to behave correctly. However, this is incompatible with smp_call_function() which will wait for online CPUs to respond to an IPI - which this one won't because interrupts are disabled. I think there was some discussion about how to fix this but I don't recall the details. thanks, Russell. then could i think this is an ARM-kernel-specific bug which exists on all ARM SMP chips for the moment? and that bug doesn't happen on x86: I don't think so. There's nothing ARM specific about it. There are few patches floating around for this issue. I posted one version long back [1] and then there was one more form Thomas G. The most recent is from one is from Peter Z [2] which is moving the fix for the cup online race to core code. Can you try Peter's patch with your test-case ? Regards, Santosh [1] https://lkml.org/lkml/2011/6/20/79 [2] https://lkml.org/lkml/2011/12/15/255 [1] is already fixed - and is not the latest problem with this code. Fixing the problem in [1] actually itself created the latest problem with smp_call_function() which wasn't there before this change. Patch [2] refers to this problem and proposes a fix for it. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Is Pandaboard cpuhotplug working stably?
On Wed, Dec 21, 2011 at 05:23:48PM +0800, Barry Song wrote: Hi guys, i tried cpuhotplug on pandaboard for both Pandroid_Froyo_L27.8.2_release_pkg and Linaro 11.11. It has failed to work stably. On Pandroid_Froyo_L27.8.2_release_pkg, unplugging cpu1 works well: # echo 0 /sys/devices/system/cpu/cpu1/online CPU1: shutdown if i enable the cpu1 again by echo 1 /sys/devices/system/cpu/cpu1/online, the system will restore to 3 random status: hang, normal, panic. Using Linaro 11.11 release, echo 0 /sys/devices/system/cpu/cpu1/online will make system hang and the whole system will not be able to reset by pressing reset key, the only way to reset system is pulling out AV power. i am sorry i can't get more time to debug and find more clues. just want to ask people whether this is a version the cpuhotplug works normal on? cpu hotplug is basically totally buggered - the preconditions placed upon the bringup code path are basically impossible to satisfy in any shape or form at the moment. There's the requirement that the secondary CPU is marked online and active before interrupts are enabled for the thread migration stuff to behave correctly. However, this is incompatible with smp_call_function() which will wait for online CPUs to respond to an IPI - which this one won't because interrupts are disabled. I think there was some discussion about how to fix this but I don't recall the details. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Is Pandaboard cpuhotplug working stably?
On Wed, Dec 21, 2011 at 05:59:07PM +0800, Barry Song wrote: 2011/12/21 Russell King - ARM Linux li...@arm.linux.org.uk: cpu hotplug is basically totally buggered - the preconditions placed upon the bringup code path are basically impossible to satisfy in any shape or form at the moment. There's the requirement that the secondary CPU is marked online and active before interrupts are enabled for the thread migration stuff to behave correctly. However, this is incompatible with smp_call_function() which will wait for online CPUs to respond to an IPI - which this one won't because interrupts are disabled. I think there was some discussion about how to fix this but I don't recall the details. thanks, Russell. then could i think this is an ARM-kernel-specific bug which exists on all ARM SMP chips for the moment? and that bug doesn't happen on x86: I don't think so. There's nothing ARM specific about it. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 3/6] clk: introduce the common clock framework
On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote: On Wed, Dec 14, 2011 at 5:18 AM, Thomas Gleixner t...@linutronix.de wrote: On Tue, 13 Dec 2011, Mike Turquette wrote: +void __clk_unprepare(struct clk *clk) +{ + if (!clk) + return; + + if (WARN_ON(clk-prepare_count == 0)) + return; + + if (--clk-prepare_count 0) + return; + + WARN_ON(clk-enable_count 0); So this leaves the clock enable count set. I'm a bit wary about that. Shouldn't it either return (including bumping the prepare_count again) or call clk_disable() ? No it should not. I've hit this in my port of OMAP. It comes from this simple situation: driver 1 (adapted for clk_prepare/clk_unprepare): clk_prepare(clk); clk_enable(clk); ... driver2 (not adapted for clk_prepare/clk_unprepare): clk_enable(clk); So this is basically buggy. Look, it's quite simple. Convert _all_ your drivers to clk_prepare/clk_unprepare _before_ you start switching your platform to use these new functions. You can do that _today_ without exception. We must refuse to merge _any_ user which does this the old way - and we should have been doing this since my commit was merged into mainline to allow drivers to be converted. And stop trying to think of ways around this inside clk_prepare/ clk_unprepare/clk_enable/clk_disable. You can't do it. Just fix _all_ the drivers. Now. Before you start implementing clk_prepare/clk_unprepare. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC V1 1/4] cpufreq: add arm soc generic cpufreq driver
On Thu, Dec 15, 2011 at 12:50:07PM -0600, Mark Langsdorf wrote: I'd prefer to see clk_get90 replaced with of_clk_get() and get_this_cpu_node() from the clk-cpufreq driver by Jamie Iles that I resubmitted yesterday. Why isn't of_clk_get() hidden inside clk_get() ? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC V1 1/4] cpufreq: add arm soc generic cpufreq driver
On Thu, Dec 15, 2011 at 07:16:35PM +0800, Richard Zhao wrote: +#ifdef CONFIG_SMP + /* loops_per_jiffy is not updated by the cpufreq core for SMP systems. + * So update it for all CPUs. + */ + for_each_possible_cpu(cpu) + per_cpu(cpu_data, cpu).loops_per_jiffy = + cpufreq_scale(per_cpu(cpu_data, cpu).loops_per_jiffy, + freqs.old, freqs.new); NAK. If you think this is a good solution, you're very wrong. If this is what's in the core cpufreq code, then it too is very broken. I've seen this exact method result in the loops_per_jiffy being totally buggered over time by the constant scaling up and down. The only way to do this _sensibly_ is to record the _initial_ loops_per_jiffy and _inital_ frequency, and scale from that. That way you get consistent results irrespective of the scalings you do over time, rather than something which continually deteriorates with every frequency change. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On Mon, Dec 05, 2011 at 02:15:56PM -0700, Paul Walmsley wrote: The types associated with clock rates in the clock interface (include/linux/clk.h) are inconsistent, and we should fix this. Rubbish. They're different with good reason. Rates are primerily unsigned quantities - and should be treated as such. The exception is clk_round_rate() which returns the rate, but also _may_ return an error. Therefore, its return type has to be signed. We could fix the immediate problem by changing the prototype of clk_round_rate() to pass the rounded rate back to the caller via a pointer in one of the arguments, and return an error code (if any) via the return value: int clk_round_rate(struct clk *clk, unsigned long rate, unsigned long *rounded_rate); Yes that might have been a better solution. But I'd propose that we instead increase the size of struct clk.rate to be s64: s64 clk_round_rate(struct clk *clk, s64 desired_rate); int clk_set_rate(struct clk *clk, s64 rate); s64 clk_get_rate(struct clk *clk); struct clk { ... s64 rate; ... }; That way the clock framework can accommodate current clock rates, as well as any conceivable future clock rate. (Some production CPUs are already running at clock rates greater than 4 GiHZ[1]. RF devices with 4 GiHz+ clock rates are also common, such as 802.11a devices running in the 5.8 GHz band, and drivers for those may eventually wish to use the clock framework.) Yuck. You are aware that 64-bit math on 32-bit CPUs sucks? So burdening _everything_ with 64-bit rate quantities is absurd. As for making then 64-bit signed quantities, that's asking for horrid code from gcc for the majority of cases. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On Fri, Dec 02, 2011 at 10:13:10AM -0700, Paul Walmsley wrote: Hi Russell, On Thu, 1 Dec 2011, Russell King - ARM Linux wrote: On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote: 1. When a clock user calls clk_enable() on a clock, the clock framework should prevent other users of the clock from changing the clock's rate. This should persist until the clock user calls clk_disable() (but see also #2 below). This will ensure that clock users can rely on the rate returned by clk_get_rate(), as long as it's called between clk_enable() and clk_disable(). And since the clock's rate is guaranteed to remain the same during this time, code that cannot tolerate clock rate changes without special handling (such as driver code for external I/O devices) will work safely without further modification. So, if you have a PLL whose parent clock is not used by anything else. You want to program it to a certain rate. You call clk_disable() on the PLL clock. The approach described wouldn't require the PLL to be disabled before changing its rate. If there are no other users of the PLL, or if the other users of the PLL have indicated that it's safe for others to change the PLL's rate, the clock framework would allow the PLL rate change, even if the PLL is enabled. (modulo any notifier activity, and assuming that the underlying PLL hardware allows its frequency to be reprogrammed while the PLL is enabled) This walks up the tree and disables the parent. You then try to set the rate using clk_set_rate(). clk_set_rate() in this circumstance can't wait for the PLL to lock because it can't - there's no reference clock for it. As an aside, this seems like a good time to mention that the behavior of clk_set_rate() on a disabled clock needs to be clarified. It's more complicated than that. Clocks have more states than just enabled and disabled. There is: - unprepared - prepared and disabled - prepared and enabled Implementations can chose at which point to enable their PLLs and wait for them to lock - but if they want to sleep while waiting, they must do this in the prepare method, not the enable method (remember, enable is to be callable from atomic contexts.) So, it's entirely possible that a prepared clock will have the PLLs up and running, which means that clk_set_rate() can also wait for the PLL to stablize (which would be a good idea.) Now... we can say that PLLs should be locked when the prepare method returns, or whenever clk_set_rate() returns. The problem with that is there's a race condition between clk_enable() and clk_set_rate() - if we allow clk_set_rate() to sleep waiting for the PLL to lock, a concurrent clk_enable() can't be prevented from returning because that would involve holding a spinlock... I think this is just one of those unsolvable issues - if you have concurrent users of a PLL there's not much you can do - race free - to prevent the PLL changing beneath some user. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On Thu, Dec 01, 2011 at 11:30:16AM -0700, Paul Walmsley wrote: The intention behind the clk_{allow,block}_rate_change() proposal was to allow the current user of the clock to change its rate without having to call clk_{allow,block}_rate_change(), if that driver was the sole user of the clock. And how does a driver know that? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
On Wed, Nov 23, 2011 at 08:59:04AM -0800, Tony Lindgren wrote: ..let's plan on getting rid of the early usage of clocks instead so you don't have the issue of deferring stuff. No - we have too many platforms already using them early to do a change like this - and to do such a change will force them to invent some other way. That's just stupid. Keep the clk API as a fundamental thing which should be initialized early so we don't have to invent new clk APIs to get around its unavailability. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 5/5] clk: export tree topology and clk data via sysfs
On Wed, Nov 23, 2011 at 10:55:19AM -0800, Tony Lindgren wrote: What else are you aware of that is really needed early for clocks other than clockevent? TWD will lose its auto-calibration. Then there's various clock source and clock event implementations. These all call for the clk API to be up and running at init_early time. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure
On Mon, Oct 17, 2011 at 01:05:39PM +0200, Sascha Hauer wrote: It's not a problem to associate multiple clocks to a device, we can do this now already. What a driver can't do now is give-me-all-clocks-I-need(dev), but this problem should not be solved at clock core level but at the clkdev level. I don't think it even needs solving at the clkdev level. In order to get all clocks for a specific device, we'd need variable length arrays to store the individual struct clk pointers, which isn't going to be all that nice. We'd need clk_get_alldev() and clk_put_alldev() to deal with these variable length arrays - and as far as the driver is concerned that's an opaque object - it can't know anything about any particular individual struct clk in the array. Then we'd need operations to operate on the array itself, which means much more API baggage. Also, if it did need to know about one particular struct clk, then there's problems with avoiding that struct clk in the alldev() versions (or we'll see drivers doing clk_get() followed by clk_disable() to work-around any enabling done via the array versions.) The fact is that the different clocks for a device are really different clocks. A dumb driver may want to request/enable all relevant clocks at once while a more sophisticated driver may want to enable the clock for accessing registers in the probe function and a baud clock on device open time. I don't think you can get away from drivers having to know about their individual clocks in some form or other - and I don't think a dumb driver should be a justification for creating an API. If a dumb driver wants to get the clocks for a device it has to behave as if it were a smart driver and request each one (maybe having an array itself) such as this: static const char *con_ids[NR_CLKS] = { foo, bar, }; struct driver_priv { struct clk *clk[NR_CLKS]; }; for (err = i = 0; i NR_CLKS; i++) { priv-clk[i] = clk_get(dev, con_ids[i]; if (IS_ERR(priv-clk[i])) { err = PTR_ERR(priv-clk[i]); break; } } if (err) { for (i = 0; i NR_CLKS !IS_ERR(priv-clk[i]); i++) clk_put(priv-clk[i]); return err; } This approach also has the advantage that we know what order the clocks will be in the array, and so we can sensibly index the array to obtain a particular clock. However, going back to the bus fabric case, a driver probably doesn't have the knowledge - it's a platform topology thing - one which can't be represented by a clock tree. It might help if we represented our busses closer to reality - like PCI does - rather than a flattened set of platform devices. Couple that with runtime PM and a driver for the bus fabric, and it should fall out fairly naturally from the infrastructure we already have. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 4/7] clk: Add simple gated clock
On Thu, Sep 22, 2011 at 03:26:59PM -0700, Mike Turquette wrote: diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c new file mode 100644 index 000..a1d8e79 --- /dev/null +++ b/drivers/clk/clk-gate.c @@ -0,0 +1,78 @@ +/* + * 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 clk gate implementation + */ + +#include linux/clk.h +#include linux/module.h +#include asm/io.h linux/io.h please. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure
On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote: struct clk_hw_ops { int (*prepare)(struct clk_hw *); void(*unprepare)(struct clk_hw *); int (*enable)(struct clk_hw *); void(*disable)(struct clk_hw *); unsigned long (*recalc_rate)(struct clk_hw *); int (*set_rate)(struct clk_hw *, unsigned long, unsigned long *); long(*round_rate)(struct clk_hw *, unsigned long); int (*set_parent)(struct clk_hw *, struct clk *); struct clk *(*get_parent)(struct clk_hw *); }; Platform clock code can register a clock through clk_register, passing a set of operations, and a pointer to hardware-specific data: struct clk_hw_foo { struct clk_hw clk; void __iomem *enable_reg; }; Eww, no, this really isn't going to scale for platforms like OMAP - to have all the operations indirected through a set of function pointers for every clock just because the enable register (or enable bit) is in a different position is completely absurd. I'm not soo concerned about the increase in memory usage (for 100 to 200 clock definitions its only 4 to 8k) but what worries me the most is initializing these damned things. It's an awful lot of initializers, which means the potential for an awful lot of conflicts should something change in this structure. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: S3C2410: Remove section mismatch warning
On Mon, Oct 03, 2011 at 03:10:41PM +0530, Tushar Behera wrote: Some of the functions and structures did not have _init or __initdata attributes, even though they were referenced from functions / structures with those attribute, resulting in section mismatches. Firstly - it's a good idea to include the warnings which you're fixing in the commit log text, so that people know exactly what is being fixed. diff --git a/arch/arm/mach-s3c2410/usb-simtec.c b/arch/arm/mach-s3c2410/usb-simtec.c index 29bd3d9..3a1028c 100644 --- a/arch/arm/mach-s3c2410/usb-simtec.c +++ b/arch/arm/mach-s3c2410/usb-simtec.c @@ -104,7 +104,7 @@ static struct s3c2410_hcd_info usb_simtec_info __initdata = { }; -int usb_simtec_init(void) +int __init usb_simtec_init(void) { int ret; This one looks fine. diff --git a/arch/arm/mach-s3c2416/irq.c b/arch/arm/mach-s3c2416/irq.c index 28ad20d..153cb2f 100644 --- a/arch/arm/mach-s3c2416/irq.c +++ b/arch/arm/mach-s3c2416/irq.c @@ -234,7 +234,7 @@ static int __init s3c2416_irq_add(struct sys_device *sysdev) return 0; } -static struct sysdev_driver s3c2416_irq_driver = { +static struct sysdev_driver s3c2416_irq_driver __initdata = { .add= s3c2416_irq_add, }; I remain entirely unconvinced that this is correct. As a result of the sysdev_driver_register(s3c2416_sysclass, s3c2416_irq_driver); call, this structure is placed on a list. If this structure is marked __initdata, then the memory behind the structure will be freed and overwritten - however, it's still on a list which might be walked. Such a walk would cause a kernel oops or might even be an exploitable security hole if that page ends up in userspace - especially as said structure contains function calls which would be called in privileged mode. The same comment applies to the other sysdev driver structures you're marking __initdata too. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure
On Mon, Oct 03, 2011 at 05:31:08PM +0100, Mark Brown wrote: I dunno, I get the impression that some of this is due to the current limitations of the clock API rather than due to a lack of clocks - perhaps that's specific to the applications I look at, though. applications The clk API per-se has nothing to do with how clocks are registered. There are two things that are the clk API: 1. clkdev - which deals with translating devices + connection IDs to struct clk. This has no ordering requirements wrt requiring parents to be initialized before their children (it has no care about that at all because it's not within its definition.) For this, registration is about connecting device + connection IDs to a struct clk. 2. the driver API, defining how the opaque struct clk is looked up, obtained and then manipulated. This has no 'registration' stuff. So, whether clocks are a tree or flat is unspecified. It's unspecified whether there's any particular order required. In fact, with a clock tree, it's entirely possible that only the leaf clocks will be 'registered' with clkdev. How the rest of the clock tree is initialized is beyond the scope of the driver clk API. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2] ASoC: omap: convert per-board modules to platform drivers
On Fri, Sep 09, 2011 at 09:11:52AM -0700, Mark Brown wrote: On Fri, Sep 09, 2011 at 10:41:56AM +0100, Russell King - ARM Linux wrote: On Thu, Sep 08, 2011 at 04:59:04PM -0700, Mark Brown wrote: The problem is that someone has to manually go and add the device to every board that needs one and people find that tedious and slightly inelegant Sheesh. So now you're arguing against your statement above? Please stop wasting my time. There's two things going on here - what we are doing and what people would like to be able to do. What we are doing is explicitly adding devices, what people would like to do is infer the devices from the board type. Personally I'm totally happy with explicitly adding an audio device, but not everyone is and I do understand where they're coming from. Well, with DT, there won't be any 'board type' anymore. There won't be any 'machine_is_xxx()' to sort it out anymore. Using DT, all that will be history - it's all got to be sorted out by either devices or device properties. So, given that, I don't see the logic of having two methods - it might as well be dealt with by devices and [platform data for non-DT | DT properties], and which then means we have everything working the same way irrespective of what the backing data for the platform actually is. Therefore, as we are heading for DT, I'd definitely say that having machine_is_xxx() outside of arch/arm is a bug, no less and no more. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3] ASoC: omap: convert per-board modules to platform drivers
On Fri, Sep 09, 2011 at 12:38:51PM +0100, Mans Rullgard wrote: This converts the per-board modules to platform drivers for a device created by in main platform setup. These drivers call snd_soc_register_card() directly instead of going via a soc-audio device and the corresponding driver in soc-core. Signed-off-by: Mans Rullgard mans.rullg...@linaro.org --- This version uses a table to assign the device name in omap_init_audio(). It is certainly less ugly than the previous patch. Again, tested only on Beagle. --- arch/arm/mach-omap2/devices.c | 34 +++ sound/soc/omap/am3517evm.c| 55 +++--- sound/soc/omap/igep0020.c | 52 +++-- sound/soc/omap/n810.c | 73 ++--- sound/soc/omap/omap3beagle.c | 55 +++--- sound/soc/omap/omap3evm.c | 56 --- sound/soc/omap/omap3pandora.c | 70 --- sound/soc/omap/overo.c| 56 +++ sound/soc/omap/rx51.c | 55 +-- sound/soc/omap/sdp3430.c | 65 ++-- sound/soc/omap/sdp4430.c | 60 + sound/soc/omap/zoom2.c| 68 +- 12 files changed, 458 insertions(+), 241 deletions(-) I don't think this is an improvement. Just look at the diffstat - it almost doubles the number of lines of code. One thing here which is utterly silly is: +static struct { + int machine; + const char *name; +} soc_device_names[] = { + { MACH_TYPE_OMAP3517EVM,am3517evm-soc-audio }, + { MACH_TYPE_IGEP0020, igep2-soc-audio }, + { MACH_TYPE_NOKIA_N810, n8x1-soc-audio}, + { MACH_TYPE_NOKIA_N810_WIMAX, n8x1-soc-audio}, + { MACH_TYPE_OMAP3_BEAGLE, omap3beagle-soc-audio }, + { MACH_TYPE_DEVKIT8000, omap3beagle-soc-audio }, + { MACH_TYPE_OMAP3EVM, omap3evm-soc-audio}, + { MACH_TYPE_OMAP3_PANDORA, pandora-soc-audio }, + { MACH_TYPE_OVERO, overo-soc-audio, }, + { MACH_TYPE_CM_T35, overo-soc-audio, }, + { MACH_TYPE_NOKIA_RX51, rx51-soc-audio, }, + { MACH_TYPE_OMAP_3430SDP, sdp3430-soc-audio,}, + { MACH_TYPE_OMAP_4430SDP, sdp4430-soc-audio,}, + { MACH_TYPE_OMAP_ZOOM2, zoom2-soc-audio, }, +}; So you're using the machine ID to select the name of the device. (That's not really DT compatible.) +static int __init am3517evm_soc_init(void) +{ + if (!machine_is_omap3517evm()) + return -ENODEV; But then you conditionalize the registration of the drivers on the platform as well. Why? It's pointless. If you don't have the core code register the struct device for this platform then this driver won't be bound to a device, and therefore the probe function won't be called. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2] ASoC: omap: convert per-board modules to platform drivers
On Fri, Sep 09, 2011 at 12:30:11PM -0700, Mark Brown wrote: On Fri, Sep 09, 2011 at 08:01:35PM +0100, Russell King - ARM Linux wrote: Well, with DT, there won't be any 'board type' anymore. There won't be any 'machine_is_xxx()' to sort it out anymore. Using DT, all that will be history - it's all got to be sorted out by either devices or device properties. There is a board type - there's a root compatible property for the board which fulfils this purpose - so the situation with and without device tree is essentially the same. So instead of a table of machine id numbers and soc device strings, you're going to have a table of board 'compatible' strings and soc device strings, and you're going to have to manually create the struct device with that name. That's just twisted and utterly insane - adding more code for precisely zero benefit what so ever. Think about it - the device tree is *already* creating platform devices for entries in the device tree file. What's the point of having this special ASoC code look up the platform compatible property in a table of strings to find a different string to manually create a device with. Why not just _add_ the bloody device to the DT file in the first place? That's partly what DT is there for - to create platform specific struct devices. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2] ASoC: omap: convert per-board modules to platform drivers
On Thu, Sep 08, 2011 at 03:29:11PM -0700, Mark Brown wrote: On Thu, 2011-09-08 at 22:28 +0200, Arnd Bergmann wrote: On Thursday 08 September 2011 20:05:48 Mans Rullgard wrote: I had the same thought, but I couldn't find a suitable string anywhere. Are you suggesting an if(machine_is_foo()) cascade in omap_init_audio()? I'll be the first to agree this patch is not particularly pretty. My general feeling is that practically every time someone writes machine_is_*(), they are doing it wrong. There are of course exceptions, but I would strongly recommend to have the initialization calling up from the board file into more general functions instead of having all boards calling the same function which then goes to board specific code again. I have to agree, that seems tasteless. I'd expect something like triggering registration of devices based off walking down a table of machine IDs or something. One other issue to consider here is that we don't want to discourage people from sharing machine drivers while we can so it can't be completely automatic, it This problem has been solved (before DT) for _ages_ through the use of platform devices in the platform support files, registered by the .init_machine callback. Where it went wrong is when ASOC and PCMCIA became rather complicated that you could no longer just pass a platform data structure, but instead needed some complex platform specific code - which started the demand to have mini platform specific chunks of code in drivers/pcmcia/ and sound/. With DT of course, all devices get instantiated from the device tree, so there should not be any more platform specific chunks of code in these locations (ha, it couldn't be solved with platform data so I suspect it will continue to persist, forever unsolved.) ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2] ASoC: omap: convert per-board modules to platform drivers
On Thu, Sep 08, 2011 at 03:47:31PM -0700, Mark Brown wrote: On Thu, Sep 08, 2011 at 11:37:20PM +0100, Russell King - ARM Linux wrote: With DT of course, all devices get instantiated from the device tree, so there should not be any more platform specific chunks of code in these locations (ha, it couldn't be solved with platform data so I suspect it will continue to persist, forever unsolved.) That's not the case at all for audio, the PCB schematic for the audio subsystem on a device like a smartphone is a sufficiently interesting piece of hardware to be a device with a driver in its own right. The ASoC machine drivers aren't about instantiating devices, they are about controlling the interrelationships between the various devices in the audio subsystem. What will happen for device tree is that there will be a device in the device tree for the ASoC board. Sounds like you just solved the machine_is_xxx() problem in ASoC land too there. If you're _already_ going for separate devices to describe the ASoC stuff on the board, then there's no reason that couldn't have already been done to eliminate the machine_is_xxx() usage in ASoC - rather than complaining about machine_is_xxx() not being a very good solution. As I said, the problem was solved years ago, and all the component parts have been there also for years. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v9 00/12] use nonblock mmc requests to minimize latency
On Sun, Aug 28, 2011 at 12:50:54PM +0200, Per Forlin wrote: On 26 August 2011 18:28, Santosh santosh.shilim...@ti.com wrote: + Balaji, On Friday 26 August 2011 09:49 PM, Russell King - ARM Linux wrote: I'm not sure who's responsible for this, but the nonblocking MMC stuff is broken on OMAPs HSMMC: mmcblk0: error -84 transferring data, sector 149201, nr 64, cmd response 0x900, card status 0xb00 mmcblk0: retrying using single block read [ cut here ] WARNING: at /home/rmk/git/linux-2.6-rmk/lib/dma-debug.c:811 check_unmap+0x1ac/0x764() omap_hsmmc omap_hsmmc.0: DMA-API: device driver tries to free DMA memory it has not allocated [device address=0x80933000] [size=20480 bytes] Modules linked in: Backtrace: [c0017874] (dump_backtrace+0x0/0x10c) from [c02ce8ac] (dump_stack+0x18/0x1c) r7:c1abfcb8 r6:c0186248 r5:c037de51 r4:032b [c02ce894] (dump_stack+0x0/0x1c) from [c0039ed4] (warn_slowpath_common+0x58/0x70) [c0039e7c] (warn_slowpath_common+0x0/0x70) from [c0039f90] (warn_slowpath_fmt+0x38/0x40) r8:c1abfd50 r7: r6:5000 r5: r4:80933000 [c0039f58] (warn_slowpath_fmt+0x0/0x40) from [c0186248] (check_unmap+0x1ac/0x764) r3:c0367d55 r2:c037e24d [c018609c] (check_unmap+0x0/0x764) from [c0186978] (debug_dma_unmap_sg+0x100/0x134) [c0186878] (debug_dma_unmap_sg+0x0/0x134) from [c0019770] (dma_unmap_sg+0x24/0x7c) [c001974c] (dma_unmap_sg+0x0/0x7c) from [c0207220] (omap_hsmmc_post_req+0x48/0x54) [c02071d8] (omap_hsmmc_post_req+0x0/0x54) from [c01fb644] (mmc_start_req+0x9c/0x128) r4:c1a76000 [c01fb5a8] (mmc_start_req+0x0/0x128) from [c02049fc] (mmc_blk_issue_rw_rq+0x80/0x460) r8:c1ab5c00 r7:0001 r6:c1ab5824 r5:c1ab5824 r4:c1ab5c00 [c020497c] (mmc_blk_issue_rw_rq+0x0/0x460) from [c02050a8] (mmc_blk_issue_rq+0x2cc/0x2fc) [c0204ddc] (mmc_blk_issue_rq+0x0/0x2fc) from [c02056c4] (mmc_queue_thread+0xa0/0x104) [c0205624] (mmc_queue_thread+0x0/0x104) from [c0054f8c] (kthread+0x88/0x90) [c0054f04] (kthread+0x0/0x90) from [c003d9a4] (do_exit+0x0/0x618) r7:0013 r6:c003d9a4 r5:c0054f04 r4:c1a7bc7c ---[ end trace 3314ad56daf5d14f ]--- Luckily thinks continue to work, but clearly releasing DMA mappings which drivers don't own is bad news. Unfortunately, I don't have the bandwidth to be able to investigate this at present - well, I could do but then I'd have to drop working on the OMAP4 vs generic suspend/resume code and learn about something I've no current clue about. Please continue your help on generic suspend. Can someone please investigate and fix whatever is broken. Will find somebody to look at this issue. I had a look at this and my guess is that the same mapped DMA memory is unmapped twice. First the memory is unmapped in omap_hsmmc_dma_cleanup() due to the mmc error, then later in omap_hsmmc_post_req(). Here is a patch to resolve that. I haven't had the chance to test it yet though. --- drivers/mmc/host/omap_hsmmc.c |7 +-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 21e4a79..7f40767 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -1011,6 +1011,7 @@ static void omap_hsmmc_dma_cleanup(struct omap_hsmmc_host *host, int errno) host-data-sg_len, omap_hsmmc_get_dma_dir(host, host-data)); omap_free_dma(dma_ch); + data-host_cookie = 0; } host-data = NULL; } @@ -1576,8 +1577,10 @@ static void omap_hsmmc_post_req(struct mmc_host *mmc, struct mmc_request *mrq, struct mmc_data *data = mrq-data; if (host-use_dma) { - dma_unmap_sg(mmc_dev(host-mmc), data-sg, data-sg_len, -omap_hsmmc_get_dma_dir(host, data)); + if (data-host_cookie) + dma_unmap_sg(mmc_dev(host-mmc), data-sg, +data-sg_len, +omap_hsmmc_get_dma_dir(host, data)); data-host_cookie = 0; I also checked the mmci code for this same issue and mmci doesn't have the same bug. MMCI works fine in 3.1-rc2 even though the code is wrong. I propose this change. @@ -529,7 +529,7 @@ static void mmci_post_request(struct mmc_host *mmc, struct mmc_request *mrq, if (chan) { if (err) dmaengine_terminate_all(chan); - if (err || data-host_cookie) + if (data-host_cookie) I'll post these patches as soon as I have managed to test them or sooner if I get a tested-by. Looking at MMCI, are you sure all those DMA engine terminate calls are correct? For example, why are we terminating all DMA engine transfers in the post request function? Surely that should've already been taken care
Re: [PATCH v9 00/12] use nonblock mmc requests to minimize latency
I'm not sure who's responsible for this, but the nonblocking MMC stuff is broken on OMAPs HSMMC: mmcblk0: error -84 transferring data, sector 149201, nr 64, cmd response 0x900, card status 0xb00 mmcblk0: retrying using single block read [ cut here ] WARNING: at /home/rmk/git/linux-2.6-rmk/lib/dma-debug.c:811 check_unmap+0x1ac/0x764() omap_hsmmc omap_hsmmc.0: DMA-API: device driver tries to free DMA memory it has not allocated [device address=0x80933000] [size=20480 bytes] Modules linked in: Backtrace: [c0017874] (dump_backtrace+0x0/0x10c) from [c02ce8ac] (dump_stack+0x18/0x1c) r7:c1abfcb8 r6:c0186248 r5:c037de51 r4:032b [c02ce894] (dump_stack+0x0/0x1c) from [c0039ed4] (warn_slowpath_common+0x58/0x70) [c0039e7c] (warn_slowpath_common+0x0/0x70) from [c0039f90] (warn_slowpath_fmt+0x38/0x40) r8:c1abfd50 r7: r6:5000 r5: r4:80933000 [c0039f58] (warn_slowpath_fmt+0x0/0x40) from [c0186248] (check_unmap+0x1ac/0x764) r3:c0367d55 r2:c037e24d [c018609c] (check_unmap+0x0/0x764) from [c0186978] (debug_dma_unmap_sg+0x100/0x134) [c0186878] (debug_dma_unmap_sg+0x0/0x134) from [c0019770] (dma_unmap_sg+0x24/0x7c) [c001974c] (dma_unmap_sg+0x0/0x7c) from [c0207220] (omap_hsmmc_post_req+0x48/0x54) [c02071d8] (omap_hsmmc_post_req+0x0/0x54) from [c01fb644] (mmc_start_req+0x9c/0x128) r4:c1a76000 [c01fb5a8] (mmc_start_req+0x0/0x128) from [c02049fc] (mmc_blk_issue_rw_rq+0x80/0x460) r8:c1ab5c00 r7:0001 r6:c1ab5824 r5:c1ab5824 r4:c1ab5c00 [c020497c] (mmc_blk_issue_rw_rq+0x0/0x460) from [c02050a8] (mmc_blk_issue_rq+0x2cc/0x2fc) [c0204ddc] (mmc_blk_issue_rq+0x0/0x2fc) from [c02056c4] (mmc_queue_thread+0xa0/0x104) [c0205624] (mmc_queue_thread+0x0/0x104) from [c0054f8c] (kthread+0x88/0x90) [c0054f04] (kthread+0x0/0x90) from [c003d9a4] (do_exit+0x0/0x618) r7:0013 r6:c003d9a4 r5:c0054f04 r4:c1a7bc7c ---[ end trace 3314ad56daf5d14f ]--- Luckily thinks continue to work, but clearly releasing DMA mappings which drivers don't own is bad news. Unfortunately, I don't have the bandwidth to be able to investigate this at present - well, I could do but then I'd have to drop working on the OMAP4 vs generic suspend/resume code and learn about something I've no current clue about. Can someone please investigate and fix whatever is broken. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH 3/4] ARM: EXYNOS4: Add support AFTR mode cpuidle state on EXYNOS4210
On Fri, Aug 19, 2011 at 06:39:59PM +0530, Amit Daniel Kachhap wrote: +ENTRY(exynos4_enter_lp) + stmfd sp!, { r3 - r12, lr } + + adr r0, sleep_save_misc + + mrc p15, 0, r2, c15, c0, 0 @ read power control register + str r2, [r0], #4 + + mrc p15, 0, r2, c15, c0, 1 @ read diagnostic register + str r2, [r0], #4 + + ldr r3, =resume_with_mmu + bl cpu_suspend + + mov r0, sp + bl exynos4_cpu_lp + + /* Restore original sp */ + mov r0, sp + add r0, r0, #4 + ldr sp, [r0] + + mov r0, #0 + b early_wakeup This is based upon old kernel code. Clearly hasn't been tested with anything later than 3.0. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5] Add ARM cpu topology definition
On Tue, Jul 05, 2011 at 02:16:23PM +0100, Russell King - ARM Linux wrote: Looks fine now, and so can go to my patch system. Many thanks. Ping. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5] Add ARM cpu topology definition
On Mon, Aug 08, 2011 at 04:42:42PM +0530, RACHANA TEWARI wrote: Does anyone know way to initiate a bash script(to run on linux box) from windows Written in C#? How to write these Scripts... This has nothing to do with CPU topology. Please do not topic hijack. This is also off-topic for these mailing lists. You may wish to consider posting your query to more appropriate mailing lists or forums (eg, those dealing with C#). Thanks. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: do not mark CPU 0 as hotpluggable
On Wed, Jul 20, 2011 at 04:32:25PM -0700, Mike Turquette wrote: A quick poll of the ARM platforms that implement CPU Hotplug support shows that every platform treats CPU 0 as a special case that cannot be hotplugged. In fact every platform has identical code for platform_cpu_die which returns -EPERM in the case of CPU 0. Are you sure that's just not because everyone copied what Realview has been doing (highly likely)? I suspect that there's no reason that CPU0 can't be taken down, especially on those platforms which don't take the CPU fully offline but just put it into a WFI loop. Those which restart the CPUs through the boot loader probably detect CPU0 as the boot CPU, so they probably can't take CPU0 down. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: do not mark CPU 0 as hotpluggable
On Thu, Jul 21, 2011 at 11:03:04AM +0530, Santosh Shilimkar wrote: Just talking on behalf of OMAP, we can't offline CPU0 and limitation would remain in future OMAPs too. Apart from the broken IRQ migration, and the way CPU0 immediately reawakes if it is offlined on OMAP4 (even when IRQs are migrated off CPU0) because omap_read_auxcoreboot0() returns 0, is there any other reason? With fixed IRQ migration and forcing CPU0 into an infinite WFI loop, I can offline CPU0 and still have a running system. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH 05/17] ARM: kernel: save/restore kernel IF
On Tue, Jul 12, 2011 at 11:12:57AM +0100, Lorenzo Pieralisi wrote: Thank you very much Russell for this recap. On Mon, Jul 11, 2011 at 07:40:10PM +0100, Russell King - ARM Linux wrote: On Mon, Jul 11, 2011 at 03:00:47PM +0100, Lorenzo Pieralisi wrote: Well, short answer is no. On SMP we do need to save CPU registers but if just one single cpu is shutdown L2 is still on. cpu_suspend saves regs on the stack that has to be cleaned from L2 before shutting a CPU down which make things more complicated than they should. Hang on. Please explain something to me here. You've mentioned a few times that cpu_suspend() can't be used because of the L2 cache. Why is this the case? OMAP appears to have code in its sleep path - which has been converted to cpu_suspend() support - to deal with the L2 issues. OMAP4, it is SMP configs I am talking about. Seriously, that's not something which really concerns me at present because suspend/resume in any form is not supported there in any form in mainline. All my comments are based on the mainline kernel. That's what I work with. Everything elsewhere is not my concern. So, let me say again. OMAP suspend/resume support _in_ _mainline_ will be converted at the next merge window. As that's only OMAP3 _in_ _mainline_ which has the need for saving state etc, that's the only OMAP code I have access to, and therefore that's the only thing which I've been able to fix. OMAP4 suspend/resume support doesn't exist in mainline and therefore doesn't exist for me. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH 05/17] ARM: kernel: save/restore kernel IF
On Mon, Jul 11, 2011 at 01:05:20PM -0700, Santosh Shilimkar wrote: (Just to add few more points on top of what Colin already commented) On 7/11/2011 11:40 AM, Russell King - ARM Linux wrote: On Mon, Jul 11, 2011 at 03:00:47PM +0100, Lorenzo Pieralisi wrote: Well, short answer is no. On SMP we do need to save CPU registers but if just one single cpu is shutdown L2 is still on. cpu_suspend saves regs on the stack that has to be cleaned from L2 before shutting a CPU down which make things more complicated than they should. Hang on. Please explain something to me here. You've mentioned a few times that cpu_suspend() can't be used because of the L2 cache. Why is this the case? OMAP appears to have code in its sleep path - which has been converted to cpu_suspend() support - to deal with the L2 issues. This part is not done yet Russell. Infact the cpu_resume() function need an update to work with L2 enable configuration. The code does deal with L2 cache enable in the resume path... However, lets recap. What we do in cpu_suspend() in order is: - Save the ABI registers onto the stack, and some private state - Save the CPU specific state onto the stack We need to disable C bit here to avoid any speculative artifacts during further operations before WFI. Which you are doing. - Flush the L1 cache - Call the platform specific suspend finisher Also finisher function should issue the WFI and just in case for some reason CPU doesn't hit the targeted low power state, finisher function takes care of things like enabling C bit, SMP bit etc. You're restoring the C bit in the non-off paths already which follow a failed WFI. You're not touching the SMP bit there though - was it ever reset? I don't think so. On resume, with the MMU and caches off: - Platform defined entry point is called, which _may_ be cpu_resume directly. - Platform initial code is executed to do whatever that requires - cpu_resume will be called - cpu_resume loads the previously saved private state - The CPU specific state is restored - Page table is modified to permit 1:1 mapping for MMU enable 1:1 idmap used here should be mapped as non-cached to avoid L2 related issues. I faced similar issue in OMAP sleep code earlier and later thanks to Colin, we got the fixed my making use of non-cached idmap. It is specified that if the main control register C bit is clear, accesses will be uncached. Whether you get cache hits or not is probably implementation dependent, and provided that the state information is cleaned from the L2 cache, it doesn't matter if we hit the L2 cached copy or the RAM copy. It's the same data. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 01/17] ARM: proc: add definition of cpu_reset for ARMv6 and ARMv7 cores
On Sun, Jul 10, 2011 at 12:00:24PM +0100, Will Deacon wrote: I've had a look at a bunch of the cpu_*_reset definitions and I can't see any reason why they wouldn't be callable with the flat mapping in place. In fact, there's a scary comment for xscale: However, that flat mapping doesn't save us, because this only covers space below PAGE_OFFSET virtual. We're executing these generally from virtual space at addresses greater than this, which means when the MMU is turned off, the instruction stream disappears. @ CAUTION: MMU turned off from this point. We count on the pipeline @ already containing those two last instructions to survive. which I think would disappear if the code was called via the ID map. Yes, and everything pre-ARMv6 fundamentally relies upon the instructions following the MCR to turn the MMU off already being in the CPUs pipeline. Pre-ARMv6 relies upon this kind of behaviour from the CPU: fetch decode execute mcr mov pc mcr nop mov pc mcr nop nop mov pc inst0 --- flushed --- where inst0 is the instruction at the target of the mov pc branch. May not be well documented in the architecture manual, but there have been documentation of this level against CPUs in the past. Maybe back in ARMv3 times, but the trick has proven to work all the way up to ARMv5, and these are of course CPU specific files rather than architecture specific. It's curious that with ARMs move to a more relaxed model, that turning the MMU off has visibly changed from a fairly weak operation to an effective strong instruction barrier. These now have visibly this behaviour: fetch decode execute mcr mov pc mcr inst0 mov pc mcr -- flushed -- mov pc* * - attempt to reload this instruction fails because MMU is now off. I'm not saying there that the pipeline is flushed (it may be) - but this represents the _observed_ behaviour. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH 05/17] ARM: kernel: save/restore kernel IF
On Sat, Jul 09, 2011 at 09:38:15AM +0100, Russell King - ARM Linux wrote: On Thu, Jul 07, 2011 at 04:50:18PM +0100, Lorenzo Pieralisi wrote: +static int late_init(void) +{ + int rc; + struct sr_cluster *cluster; + int cluster_index, cpu_index = sr_platform_get_cpu_index(); Stop this madness, and use the standard linux APIs like smp_processor_id here. It might actually help you find bugs, like the fact that you're in a preemptible context here, and so could be rescheduled onto any other CPU in the system _after_ you've read the MPIDR register. + + cluster_index = sr_platform_get_cluster_index(); + cluster = main_table.cluster_table + cluster_index; + main_table.os_mmu_context[cluster_index][cpu_index] = + current-active_mm-pgd; + cpu_switch_mm(main_table.fw_mmu_context, current-active_mm); + rc = sr_platform_init(); + cpu_switch_mm(main_table.os_mmu_context[cluster_index][cpu_index], + current-active_mm); CPU numbers are unique in the system, why do you need a 'cluster_index' to save this? In fact why do you even need to save it in a structure at all? Plus, cluster is not used, please get rid of it. Oh, that's another thing. This thread is about introducing idle support not cluster support. Cluster support is surely something different (esp. as it seems from the above code that you're trying to support several clusters of MPCore CPUs, each with physical 0-N CPU numbers.) Cluster support should be an entirely separate patch series. If that is not what this cluster stuff in this patch is about, then it's just written badly and reinforces the need for it to be rewritten - in that case there's no need for a 2D array. In any case, smp_processor_id() will be (and must be) unique in any given running kernel across all CPUs, even if you have clusters of N CPUs all physically numbered 0-N. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH 06/17] ARM: kernel: save/restore generic infrastructure
The idea of splitting a large patch up into smaller patches is to do it in a logical way so that: 1. Each patch is self-contained, adding a single new - and where possible complete - feature or bug fix. 2. Ease of review. Carving your big patch up by file does not do either of this, because all patches interact with each other. There's people within ARM Ltd who have been dealing with patch sets for some time who you could ask advice from - or who you could ask to review your patches before you send them out on the mailing list. On Thu, Jul 07, 2011 at 04:50:19PM +0100, Lorenzo Pieralisi wrote: diff --git a/arch/arm/kernel/sr.h b/arch/arm/kernel/sr.h new file mode 100644 index 000..6b24e53 --- /dev/null +++ b/arch/arm/kernel/sr.h @@ -0,0 +1,162 @@ +#define SR_NR_CLUSTERS 1 + +#define STACK_SIZE 512 + +#define CPU_A5 0x4100c050 Not used in this patch - should it be in some other patch? +#define CPU_A8 0x4100c080 Not used in this patch - should it be in some other patch? +#define CPU_A9 0x410fc090 Not used in this patch - and if this is generic code then it should not be specific to any CPU. Please get rid of all these definitions, and if they're used you need to rework these patches to have proper separation of generic code from CPU specific code. +#define L2_DATA_SIZE 16 Not used in this patch. +#define CONTEXT_SPACE_UNCACHED (2 * PAGE_SIZE) +#define PA(f) ((typeof(f) *)virt_to_phys((void *)f)) This is just messed up. Physical addresses aren't pointers, they're numeric. PA() is not used in this patch either, so it appears it can be deleted. +extern int lookup_arch(void); This doesn't exist in this patch, should it be in another patch or deleted? +/* + * Global variables + */ +extern struct sr_main_table main_table; +extern unsigned long idle_save_context; +extern unsigned long idle_restore_context; +extern unsigned long idle_mt; +extern void *context_memory_uncached; Why does this need to be a global? + +/* + * Context save/restore + */ +typedef u32 (sr_save_context_t) + (struct sr_cluster *, + struct sr_cpu*, u32); Fits on one line so should be on one line. +typedef u32 (sr_restore_context_t) + (struct sr_cluster *, + struct sr_cpu*); Fits on one line so should be on one line. + +extern sr_save_context_t sr_save_context; This doesn't exist in this patch, should it be in another patch? +extern sr_restore_context_t sr_restore_context; Ditto. + + +extern struct sr_arch *get_arch(void); Ditto. + + +/* + * 1:1 mappings + */ + +extern int linux_sr_setup_translation_tables(void); Ditto. + +/* + * dumb memory allocator + */ + +extern void *get_memory(unsigned int size); + +/* + * Entering/Leaving C-states function entries + */ + +extern int sr_platform_enter_cstate(unsigned cpu_index, struct sr_cpu *cpu, + struct sr_cluster *cluster); +extern int sr_platform_leave_cstate(unsigned cpu_index, struct sr_cpu *cpu, + struct sr_cluster *cluster); See comment at the bottom - would inline functions here be better or maybe even place them at the callsite to make the code easier to understand if they're only used at one location. + +/* save/restore main table */ +extern struct sr_main_table main_table; Why do we have two 'main_table' declarations in this same header file? + +/* + * Init functions + */ + +extern int sr_platform_runtime_init(void); Not defined in this patch. +extern int sr_platform_init(void); +extern int sr_context_init(void); + + +/* + * v7 specific + */ + +extern char *cpu_v7_suspend_size; No - stop going underneath the covers of existing APIs to get what you want. Use cpu_suspend() and cpu_resume() directly. Note that they've changed to be more flexible and those patches have been on this mailing list, and will be going in for 3.1. If they still don't do what you need, I'm going to be *pissed* because you've obviously known that they don't yet you haven't taken the time to get involved on this mailing list with the discussions over it. +extern void scu_cpu_mode(void __iomem *base, int state); Not defined in this patch - should it be in another patch or deleted? + +/* + * These arrays keep suitable stack pointers for CPUs. + * + * The memory must be 8-byte aligned. + */ + +extern unsigned long platform_cpu_stacks[CONFIG_NR_CPUS]; Ditto. +extern unsigned long platform_cpu_nc_stacks[CONFIG_NR_CPUS]; Ditto. And should these be per-cpu variables? In any case, CONFIG_NR_CPUS doesn't look right here, NR_CPUS is probably what you want. +#endif diff --git a/arch/arm/kernel/sr_context.c b/arch/arm/kernel/sr_context.c new file mode 100644 index 000..25eaa43 --- /dev/null +++ b/arch/arm/kernel/sr_context.c @@ -0,0 +1,23 @@ +/* + * Copyright (C) 2008-2011 ARM Limited + * Author(s): Jon Callan, Lorenzo Pieralisi + * + * This program is
Re: [PATCH 01/17] ARM: proc: add definition of cpu_reset for ARMv6 and ARMv7 cores
On Thu, Jul 07, 2011 at 04:50:14PM +0100, Lorenzo Pieralisi wrote: From: Will Deacon will.dea...@arm.com This patch adds simple definitions of cpu_reset for ARMv6 and ARMv7 cores, which disable the MMU via the SCTLR. This really needs fixing properly, so that we have this well defined across all supported ARM cores. Requiring ARMv6 and ARMv7 to have this code called with a flat mapping (which may overlap a section boundary) vs ARMv5 and lower code which doesn't is just silly. With any API, we need consistency. So if ARMv6 and v7 require a flat mapping, we need to ensure that ARMv5 and lower is happy to deal with that code being also called with a flat mapping. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 02/17] ARM: Add cpu power management notifiers
On Thu, Jul 07, 2011 at 04:50:15PM +0100, Lorenzo Pieralisi wrote: During some CPU power modes entered during idle, hotplug and suspend, peripherals located in the CPU power domain, such as the GIC and VFP, may be powered down. Add a notifier chain that allows drivers for those peripherals to be notified before and after they may be reset. I defer comment on this until I see what the CPU_COMPLEX_* stuff is used for. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 03/17] ARM: gic: Use cpu pm notifiers to save gic state
On Thu, Jul 07, 2011 at 04:50:16PM +0100, Lorenzo Pieralisi wrote: From: Colin Cross ccr...@android.com When the cpu is powered down in a low power mode, the gic cpu interface may be reset, and when the cpu complex is powered down, the gic distributor may also be reset. This patch uses CPU_PM_ENTER and CPU_PM_EXIT notifiers to save and restore the gic cpu interface registers, and the CPU_COMPLEX_PM_ENTER and CPU_COMPLEX_PM_EXIT notifiers to save and restore the gic distributor registers. Signed-off-by: Colin Cross ccr...@android.com Lost attributations and original author. This is based in part on code from Gary King, according to patch 6646/1 in the patch system. Moreover, how now that we have genirq dealing with the suspend/restore issues, how much of this is actually required. And should this be GIC specific or should there be a way of asking genirq to take care of some of this for us? We need to _reduce_ the amount of code needed to support this stuff, and if core code almost-but-not-quite does what we need then we need to talk to the maintainers of that code to see whether it can be changed. Because adding 212 lines to save and restore the state of every interrupt controller that we may have just isn't on. We need this properly abstracted and dealt with in a generic way. --- arch/arm/common/gic.c | 212 + 1 files changed, 212 insertions(+), 0 deletions(-) diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c index 4ddd0a6..8d62e07 100644 --- a/arch/arm/common/gic.c +++ b/arch/arm/common/gic.c @@ -29,6 +29,7 @@ #include linux/cpumask.h #include linux/io.h +#include asm/cpu_pm.h #include asm/irq.h #include asm/mach/irq.h #include asm/hardware/gic.h @@ -42,6 +43,17 @@ struct gic_chip_data { unsigned int irq_offset; void __iomem *dist_base; void __iomem *cpu_base; +#ifdef CONFIG_CPU_PM + u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)]; + u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)]; + u32 saved_spi_pri[DIV_ROUND_UP(1020, 4)]; + u32 saved_spi_target[DIV_ROUND_UP(1020, 4)]; + u32 __percpu *saved_ppi_enable; + u32 __percpu *saved_ppi_conf; + u32 __percpu *saved_ppi_pri; +#endif + + unsigned int gic_irqs; }; /* @@ -283,6 +295,8 @@ static void __init gic_dist_init(struct gic_chip_data *gic, if (gic_irqs 1020) gic_irqs = 1020; + gic-gic_irqs = gic_irqs; + /* * Set all global interrupts to be level triggered, active low. */ @@ -350,6 +364,203 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic) writel_relaxed(1, base + GIC_CPU_CTRL); } +#ifdef CONFIG_CPU_PM +/* + * Saves the GIC distributor registers during suspend or idle. Must be called + * with interrupts disabled but before powering down the GIC. After calling + * this function, no interrupts will be delivered by the GIC, and another + * platform-specific wakeup source must be enabled. + */ +static void gic_dist_save(unsigned int gic_nr) +{ + unsigned int gic_irqs; + void __iomem *dist_base; + int i; + + if (gic_nr = MAX_GIC_NR) + BUG(); + + gic_irqs = gic_data[gic_nr].gic_irqs; + dist_base = gic_data[gic_nr].dist_base; + + if (!dist_base) + return; + + for (i = 0; i DIV_ROUND_UP(gic_irqs, 16); i++) + gic_data[gic_nr].saved_spi_conf[i] = + readl_relaxed(dist_base + GIC_DIST_CONFIG + i * 4); + + for (i = 0; i DIV_ROUND_UP(gic_irqs, 4); i++) + gic_data[gic_nr].saved_spi_pri[i] = + readl_relaxed(dist_base + GIC_DIST_PRI + i * 4); + + for (i = 0; i DIV_ROUND_UP(gic_irqs, 4); i++) + gic_data[gic_nr].saved_spi_target[i] = + readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4); + + for (i = 0; i DIV_ROUND_UP(gic_irqs, 32); i++) + gic_data[gic_nr].saved_spi_enable[i] = + readl_relaxed(dist_base + GIC_DIST_ENABLE_SET + i * 4); + + writel_relaxed(0, dist_base + GIC_DIST_CTRL); +} + +/* + * Restores the GIC distributor registers during resume or when coming out of + * idle. Must be called before enabling interrupts. If a level interrupt + * that occured while the GIC was suspended is still present, it will be + * handled normally, but any edge interrupts that occured will not be seen by + * the GIC and need to be handled by the platform-specific wakeup source. + */ +static void gic_dist_restore(unsigned int gic_nr) +{ + unsigned int gic_irqs; + unsigned int i; + void __iomem *dist_base; + + if (gic_nr = MAX_GIC_NR) + BUG(); + + gic_irqs = gic_data[gic_nr].gic_irqs; + dist_base = gic_data[gic_nr].dist_base; + + if (!dist_base) + return; + + writel_relaxed(0, dist_base + GIC_DIST_CTRL); + +
Re: [PATCH 04/17] ARM: vfp: Use cpu pm notifiers to save vfp state
On Thu, Jul 07, 2011 at 04:50:17PM +0100, Lorenzo Pieralisi wrote: From: Colin Cross ccr...@android.com When the cpu is powered down in a low power mode, the vfp registers may be reset. This patch uses CPU_PM_ENTER and CPU_PM_EXIT notifiers to save and restore the cpu's vfp registers. Signed-off-by: Colin Cross ccr...@android.com --- arch/arm/vfp/vfpmodule.c | 40 1 files changed, 40 insertions(+), 0 deletions(-) diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c index f25e7ec..6f08dbe 100644 --- a/arch/arm/vfp/vfpmodule.c +++ b/arch/arm/vfp/vfpmodule.c @@ -21,6 +21,7 @@ #include asm/cputype.h #include asm/thread_notify.h #include asm/vfp.h +#include asm/cpu_pm.h #include vfpinstr.h #include vfp.h @@ -169,6 +170,44 @@ static struct notifier_block vfp_notifier_block = { .notifier_call = vfp_notifier, }; +#ifdef CONFIG_CPU_PM +static int vfp_cpu_pm_notifier(struct notifier_block *self, unsigned long cmd, + void *v) +{ + u32 fpexc = fmrx(FPEXC); + unsigned int cpu = smp_processor_id(); + + switch (cmd) { + case CPU_PM_ENTER: + if (last_VFP_context[cpu]) { + fmxr(FPEXC, fpexc | FPEXC_EN); + vfp_save_state(last_VFP_context[cpu], fpexc); + /* force a reload when coming back from idle */ + last_VFP_context[cpu] = NULL; + fmxr(FPEXC, fpexc ~FPEXC_EN); + } + break; This doesn't look right. On SMP setups, we always save the state of an enabled VFP on thread switches. That means the saved context in every thread is always up to date for all threads, except _possibly_ for the currently executing thread on the CPU. On UP setups, we only save the state when we need to, so we need to do something like the above. However, we're growing more and more functions in the VFP code dealing with saving and restoring state, and it's starting to become really silly and confusing about which function is called for what and why, and why it's different from another function doing something similar. We need to sort this out so we have a _sane_ approach to this, rather than inventing more and more creative ways to save VFP state and restore it later. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 04/17] ARM: vfp: Use cpu pm notifiers to save vfp state
On Sat, Jul 09, 2011 at 11:44:08AM +0100, Russell King - ARM Linux wrote: We need to sort this out so we have a _sane_ approach to this, rather than inventing more and more creative ways to save VFP state and restore it later. And here, let's prove that the current code is just soo bloody complex that it needs redoing. In the following code, 'last_VFP_context' is renamed to 'vfp_current_hw_state' for clarity. void vfp_sync_hwstate(struct thread_info *thread) { unsigned int cpu = get_cpu(); /* * If the thread we're interested in is the current owner of the * hardware VFP state, then we need to save its state. */ if (vfp_current_hw_state[cpu] == thread-vfpstate) { u32 fpexc = fmrx(FPEXC); /* * Save the last VFP state on this CPU. */ fmxr(FPEXC, fpexc | FPEXC_EN); vfp_save_state(thread-vfpstate, fpexc | FPEXC_EN); fmxr(FPEXC, fpexc); } Here, 'thread' is the thread we're interested in ensuring that we have up to date context in thread-vfpstate. On entry to this function, we can be running on any CPU in the system, and 'thread' could have been running on any other CPU in the system. What this code is saying is: if the currrent CPU's hardware VFP state was owned by this thread, then update the current VFP state. So far it looks sane. Now, lets consider what with thread migration. First, lets define three threads. Thread 1, we'll call 'interesting_thread' which is a thread which is running on CPU0, using VFP (so vfp_current_hw_state[0] = interesting_thread-vfpstate) and gets migrated off to CPU1, where it continues execution of VFP instructions. Thread 2, we'll call 'new_cpu0_thread' which is the thread which takes over on CPU0. This has also been using VFP, and last used VFP on CPU0, but doesn't use it again. The following code will be executed twice: cpu = thread-cpu; /* * On SMP, if VFP is enabled, save the old state in * case the thread migrates to a different CPU. The * restoring is done lazily. */ if ((fpexc FPEXC_EN) vfp_current_hw_state[cpu]) { vfp_save_state(vfp_current_hw_state[cpu], fpexc); vfp_current_hw_state[cpu]-hard.cpu = cpu; } /* * Thread migration, just force the reloading of the * state on the new CPU in case the VFP registers * contain stale data. */ if (thread-vfpstate.hard.cpu != cpu) vfp_current_hw_state[cpu] = NULL; The first execution will be on CPU0 to switch away from 'interesting_thread'. interesting_thread-cpu will be 0. So, vfp_current_hw_state[0] points at interesting_thread-vfpstate. The hardware state will be saved, along with the CPU number (0) that it was executing on. 'thread' will be 'new_cpu0_thread' with new_cpu0_thread-cpu = 0. Also, because it was executing on CPU0, new_cpu0_thread-vfpstate.hard.cpu = 0, and so the thread migration check is not triggered. This means that vfp_current_hw_state[0] remains pointing at interesting_thread. The second execution will be on CPU1 to switch _to_ 'interesting_thread'. So, 'thread' will be 'interesting_thread' and interesting_thread-cpu now will be 1. The previous thread executing on CPU1 is not relevant to this so we shall ignore that. We get to the thread migration check. Here, we discover that interesting_thread-vfpstate.hard.cpu = 0, yet interesting_thread-cpu is now 1, indicating thread migration. We set vfp_current_hw_state[1] to NULL. So, at this point vfp_current_hw_state[] contains the following: [0] = interesting_thread [1] = NULL Our interesting thread now executes a VFP instruction, takes a fault which loads the state into the VFP hardware. Now, through the assembly we now have: [0] = interesting_thread [1] = interesting_thread CPU1 stops due to ptrace (and so saves its VFP state) using the thread switch code above), and CPU0 calls vfp_sync_hwstate(). if (vfp_current_hw_state[cpu] == thread-vfpstate) { vfp_save_state(thread-vfpstate, fpexc | FPEXC_EN); BANG, we corrupt interesting_thread's VFP state by overwriting the more up-to-date state saved by CPU1 with the old VFP state from CPU0. I think this is not the only problem with this code, and it's in desperate need of being cleaned up. Until such time, adding more state saving code is just going to be extremely hairy. Finally, as far as saving state for _idle_ goes (in other words, while the CPU's idle loop in cpu_idle() is running), take a moment to consider the following: the idle thread being a kernel thread does not use VFP. It has no useful VFP state. So: 1. On SMP, because we've switched away from any userland thread, we have
Re: [PATCH 03/17] ARM: gic: Use cpu pm notifiers to save gic state
On Sat, Jul 09, 2011 at 04:01:19PM -0700, Colin Cross wrote: On Sat, Jul 9, 2011 at 3:33 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Sat, Jul 09, 2011 at 03:10:56PM -0700, Colin Cross wrote: This is necessary for cpuidle states that lose the GIC registers, not just suspend, because the GIC is in the cpu's power domain. We could avoid saving and restoring all the GIC registers in suspend and idle by reusing the initialization functions, and then having the core irq code call the unmask, set_type, and set_affinity functions on each irq to reconfigure it, but that will be very inefficient - it will convert each register write in the restore functions to a read-modify-write per interrupt in that register. Santosh is already complaining that this commong GIC restore code will be slower than the automatic DMA to restore the GIC registers that OMAP4 supports. Well, we need to come up with something sensible - a way of doing this which doesn't require every interrupt controller driver (of which we as an architecture have many) to have lots of support added. If the current way is inefficient and is noticably so, then let's talk to Thomas about finding a way around that - maybe having the generic code make one suspend/resume callback per irq gc chip rather than doing it per-IRQ. We can then reuse the same paths for suspend/resume as for idle state saving. Are you referring to moving the gic driver to be gc chip? Otherwise, I don't understand your suggestion - how is callback per chip any different than what this patch implements? It just gets it's notification through a cpu_pm notifier, which works in idle and suspend, instead of a syscore op like the gc driver does. This patch does save and restore some registers that are never modified after init, so they don't need to be saved. The point is that we should aim to get to the point where, if an interrupt controller supports PM, then it supports _all_ PM out the box and doesn't require additional code for cpu idle PM vs system suspend PM. In other words, all we should need to do is provide genirq with a couple of functions for 'save state' and 'restore state'. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH 00/17] ARM: common idle infrastructure
On Thu, Jul 07, 2011 at 04:50:13PM +0100, Lorenzo Pieralisi wrote: This patchset is a first attempt at providing a consolidation of idle code for the ARM processor architecture and a request for comment on the provided methodology. It relies and it is based on kernel features such as suspend/resume, pm notifiers and common code for cpu_reset(). Please delay this for the next merge window - there's too much going on at the moment to even start looking at this patch set. Thanks. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5] Add ARM cpu topology definition
Looks fine now, and so can go to my patch system. Many thanks. On Mon, Jul 04, 2011 at 06:49:45PM +0200, Vincent Guittot wrote: The affinity between ARM processors is defined in the MPIDR register. We can identify which processors are in the same cluster, and which ones have performance interdependency. We can define the cpu topology of ARM platform, that is then used by sched_mc and sched_smt. The default state of sched_mc and sched_smt config is disable. When enabled, the behavior of the scheduler can be modified with sched_mc_power_savings and sched_smt_power_savings sysfs interfaces. Changes since v4 : * Remove unnecessary parentheses and blank lines Changes since v3 : * Update the format of printk message * Remove blank line Changes since v2 : * Update the commit message and some comments Changes since v1 : * Update the commit message * Add read_cpuid_mpidr in arch/arm/include/asm/cputype.h * Modify header of arch/arm/kernel/topology.c * Modify tests and manipulation of MPIDR's bitfields * Modify the place and dependancy of the config * Modify Noop functions Signed-off-by: Vincent Guittot vincent.guit...@linaro.org Reviewed-by: Amit Kucheria amit.kuche...@linaro.org --- arch/arm/Kconfig| 25 +++ arch/arm/include/asm/cputype.h |6 ++ arch/arm/include/asm/topology.h | 33 + arch/arm/kernel/Makefile|1 + arch/arm/kernel/smp.c |5 ++ arch/arm/kernel/topology.c | 148 +++ 6 files changed, 218 insertions(+), 0 deletions(-) create mode 100644 arch/arm/kernel/topology.c diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 9adc278..f327e55 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1344,6 +1344,31 @@ config SMP_ON_UP If you don't know what to do here, say Y. +config ARM_CPU_TOPOLOGY + bool Support cpu topology definition + depends on SMP CPU_V7 + default y + help + Support ARM cpu topology definition. The MPIDR register defines + affinity between processors which is then used to describe the cpu + topology of an ARM System. + +config SCHED_MC + bool Multi-core scheduler support + depends on ARM_CPU_TOPOLOGY + help + Multi-core scheduler support improves the CPU scheduler's decision + making when dealing with multi-core CPU chips at a cost of slightly + increased overhead in some places. If unsure say N here. + +config SCHED_SMT + bool SMT scheduler support + depends on ARM_CPU_TOPOLOGY + help + Improves the CPU scheduler's decision making when dealing with + MultiThreading at a cost of slightly increased overhead in some + places. If unsure say N here. + config HAVE_ARM_SCU bool depends on SMP diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h index cd4458f..cb47d28 100644 --- a/arch/arm/include/asm/cputype.h +++ b/arch/arm/include/asm/cputype.h @@ -8,6 +8,7 @@ #define CPUID_CACHETYPE 1 #define CPUID_TCM2 #define CPUID_TLBTYPE3 +#define CPUID_MPIDR 5 #define CPUID_EXT_PFR0 c1, 0 #define CPUID_EXT_PFR1 c1, 1 @@ -70,6 +71,11 @@ static inline unsigned int __attribute_const__ read_cpuid_tcmstatus(void) return read_cpuid(CPUID_TCM); } +static inline unsigned int __attribute_const__ read_cpuid_mpidr(void) +{ + return read_cpuid(CPUID_MPIDR); +} + /* * Intel's XScale3 core supports some v6 features (supersections, L2) * but advertises itself as v5 as it does not support the v6 ISA. For diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h index accbd7c..a7e457e 100644 --- a/arch/arm/include/asm/topology.h +++ b/arch/arm/include/asm/topology.h @@ -1,6 +1,39 @@ #ifndef _ASM_ARM_TOPOLOGY_H #define _ASM_ARM_TOPOLOGY_H +#ifdef CONFIG_ARM_CPU_TOPOLOGY + +#include linux/cpumask.h + +struct cputopo_arm { + int thread_id; + int core_id; + int socket_id; + cpumask_t thread_sibling; + cpumask_t core_sibling; +}; + +extern struct cputopo_arm cpu_topology[NR_CPUS]; + +#define topology_physical_package_id(cpu)(cpu_topology[cpu].socket_id) +#define topology_core_id(cpu)(cpu_topology[cpu].core_id) +#define topology_core_cpumask(cpu) (cpu_topology[cpu].core_sibling) +#define topology_thread_cpumask(cpu) (cpu_topology[cpu].thread_sibling) + +#define mc_capable() (cpu_topology[0].socket_id != -1) +#define smt_capable()(cpu_topology[0].thread_id != -1) + +void init_cpu_topology(void); +void store_cpu_topology(unsigned int cpuid); +const struct cpumask *cpu_coregroup_mask(unsigned int cpu); + +#else + +static inline void init_cpu_topology(void) { } +static inline void store_cpu_topology(unsigned int cpuid) { } + +#endif + #include asm-generic/topology.h #endif /* _ASM_ARM_TOPOLOGY_H
Re: [PATCH v6 00/11] mmc: use nonblock mmc requests to minimize latency
On Tue, Jun 28, 2011 at 09:22:20AM +0300, saeed bishara wrote: On Mon, Jun 27, 2011 at 2:02 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Mon, Jun 27, 2011 at 01:34:48PM +0300, saeed bishara wrote: Russell, I'm curious about the correctness of this patch for systems with outer cache. shouldn't the dsb be issued before the outer cache maintenance? Maybe we should do two passes over SG lists then - one for the inner and another for the outer cache? In effect we could do three passes: 1. Calculate the total size of the SG list to determine whether full cache flush is more efficient. 2. Flush inner cache Then dsb() 3. Flush outer cache Another dsb() looking at l2x0 cache, it seems to me that it would be possible to do asynchronous l2 cache maintenance for range operations, so maybe that can be utilized in order to parallelism between inner cache and outer cache maintenance, so the flow can be: 2. Flush sg buffer from inner cache 3. dsb() 4. start Flush outer cache for that buffer 5. Flush next sg buffer from inner cache 6. dsb 7. goto 4. 8. when no more buffers left, wait for outer cache operations I'm not sure how practical that is given the architecture of the L2x0 controllers - where we need to wait for the previous operation to complete by reading the operation specific register and then issue a sync. Having looked at this again, I think trying to do any optimization of this code will be fraught, because of the dmabounce stuff getting in the way. If only dmabounce didn't exist... if only crap hardware didn't exist... dmabounce really needs to die. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v8 00/12] use nonblock mmc requests to minimize latency
On Fri, Jul 01, 2011 at 06:44:43PM +0200, Arnd Bergmann wrote: On Thursday 30 June 2011, Russell King - ARM Linux wrote: We've been here before - with PCMCIA's card insertion code, where you have to go through a sequence of events (insert, power up, reset, etc). The PCMCIA code used to have a collection of small functions to do each step, one chained after the other in a state machine fashion. The result was horrid. That's exactly what you'll end up with here. Threads have their place, and this is one of them. Ok, fair enough. The performance enhancement is certainly here already with getting the cache management operations out of the hot path, and for the fully asynchronous case it's not getting better by trying to be smarter. At least for ARM, the overhead of the DMA mapping operations will dwarf the overhead of the extra context switches for the foreseeable future, so we don't need to bother. Things might be different for coherent low-end CPU cores like Atom when mmc device become much faster and block access becomes CPU bound. One other thing to be considered here is whether this idea should be limited to just MMC or whether it should be extended further, to move the DMA mapping stuff out of the hot path for other block devices too. There are ARM systems with SATA which do 28MB/s - which could be improved by this technique. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4] Add ARM cpu topology definition
On Fri, Jul 01, 2011 at 07:43:30AM +0200, Vincent Guittot wrote: Changes since v3 : * Update the format of printk message * Remove blank line Can I trouble you to check the patch for more instances of the 'blank line at end of function' thing... Also, let's get rid of unnecessary parens. diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h index accbd7c..63a7454 100644 --- a/arch/arm/include/asm/topology.h +++ b/arch/arm/include/asm/topology.h @@ -1,6 +1,39 @@ #ifndef _ASM_ARM_TOPOLOGY_H #define _ASM_ARM_TOPOLOGY_H +#ifdef CONFIG_ARM_CPU_TOPOLOGY + +#include linux/cpumask.h + +struct cputopo_arm { + int thread_id; + int core_id; + int socket_id; + cpumask_t thread_sibling; + cpumask_t core_sibling; +}; + +extern struct cputopo_arm cpu_topology[NR_CPUS]; + +#define topology_physical_package_id(cpu)(cpu_topology[cpu].socket_id) +#define topology_core_id(cpu)(cpu_topology[cpu].core_id) +#define topology_core_cpumask(cpu) ((cpu_topology[cpu].core_sibling)) +#define topology_thread_cpumask(cpu) ((cpu_topology[cpu].thread_sibling)) The inner-most parens aren't required. + +#define mc_capable() (cpu_topology[0].socket_id != -1) +#define smt_capable()(cpu_topology[0].thread_id != -1) + +void init_cpu_topology(void); +void store_cpu_topology(unsigned int cpuid); +const struct cpumask *cpu_coregroup_mask(unsigned int cpu); + +#else + +static inline void init_cpu_topology(void) { }; +static inline void store_cpu_topology(unsigned int cpuid) { }; Functions don't need a ; after the } + +#endif + #include asm-generic/topology.h #endif /* _ASM_ARM_TOPOLOGY_H */ diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile index a5b31af..816a481 100644 --- a/arch/arm/kernel/Makefile +++ b/arch/arm/kernel/Makefile @@ -61,6 +61,7 @@ obj-$(CONFIG_IWMMXT)+= iwmmxt.o obj-$(CONFIG_CPU_HAS_PMU)+= pmu.o obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o AFLAGS_iwmmxt.o := -Wa,-mcpu=iwmmxt +obj-$(CONFIG_ARM_CPU_TOPOLOGY) += topology.o ifneq ($(CONFIG_ARCH_EBSA110),y) obj-y += io.o diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index 344e52b..3e8dc3b 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -31,6 +31,7 @@ #include asm/cacheflush.h #include asm/cpu.h #include asm/cputype.h +#include asm/topology.h #include asm/mmu_context.h #include asm/pgtable.h #include asm/pgalloc.h @@ -268,6 +269,9 @@ static void __cpuinit smp_store_cpu_info(unsigned int cpuid) struct cpuinfo_arm *cpu_info = per_cpu(cpu_data, cpuid); cpu_info-loops_per_jiffy = loops_per_jiffy; + + store_cpu_topology(cpuid); + Don't need this blank line. } /* @@ -354,6 +358,8 @@ void __init smp_prepare_cpus(unsigned int max_cpus) { unsigned int ncores = num_possible_cpus(); + init_cpu_topology(); + smp_store_cpu_info(smp_processor_id()); /* diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c new file mode 100644 index 000..e8f3b95 --- /dev/null +++ b/arch/arm/kernel/topology.c @@ -0,0 +1,149 @@ +/* + * arch/arm/kernel/topology.c + * + * Copyright (C) 2011 Linaro Limited. + * Written by: Vincent Guittot + * + * based on arch/sh/kernel/topology.c + * + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file COPYING in the main directory of this archive + * for more details. + */ + +#include linux/cpu.h +#include linux/cpumask.h +#include linux/init.h +#include linux/percpu.h +#include linux/node.h +#include linux/nodemask.h +#include linux/sched.h + +#include asm/cputype.h +#include asm/topology.h + +#define MPIDR_SMP_BITMASK (0x3 30) +#define MPIDR_SMP_VALUE (0x2 30) + +#define MPIDR_MT_BITMASK (0x1 24) + +/* + * These masks reflect the current use of the affinity levels. + * The affinity level can be up to 16 bits according to ARM ARM + */ + +#define MPIDR_LEVEL0_MASK 0x3 +#define MPIDR_LEVEL0_SHIFT 0 + +#define MPIDR_LEVEL1_MASK 0xF +#define MPIDR_LEVEL1_SHIFT 8 + +#define MPIDR_LEVEL2_MASK 0xFF +#define MPIDR_LEVEL2_SHIFT 16 + +struct cputopo_arm cpu_topology[NR_CPUS]; + +const struct cpumask *cpu_coregroup_mask(unsigned int cpu) +{ + return (cpu_topology[cpu].core_sibling); These parens aren't required. +} + +/* + * store_cpu_topology is called at boot when only one cpu is running + * and with the mutex cpu_hotplug.lock locked, when several cpus have booted, + * which prevents simultaneous write access to cpu_topology array + */ +void store_cpu_topology(unsigned int cpuid) +{ + struct cputopo_arm *cpuid_topo = (cpu_topology[cpuid]); Neither are these. + unsigned int mpidr; + unsigned int cpu; + + /* If the cpu topology has been already set, just return */ + if (cpuid_topo-core_id
Re: [PATCH v8 00/12] use nonblock mmc requests to minimize latency
On Thu, Jun 30, 2011 at 03:12:46PM +0200, Arnd Bergmann wrote: I think this looks good enough to merge into the linux-mmc tree, the code is clean and the benefits are clear. Acked-by: Arnd Bergmann a...@arndb.de One logical follow-up as both a cleanup and performance optimization would be to get rid of the mmc_queue_thread completely. When mmc_blk_issue_rq() is non-blocking always, you can call it directly from the mmc_request() function, instead of waking up another thread to do it for you. It isn't anywhere near that - because you need to wait for the request to complete, then analyze the results and if there has been an error, send more commands and wait for their response. To do all that in an asynchronous fashion will just create a mess of small little functions with hard to understand code. It's far better to do all that in a clear procedural way in a thread. We've been here before - with PCMCIA's card insertion code, where you have to go through a sequence of events (insert, power up, reset, etc). The PCMCIA code used to have a collection of small functions to do each step, one chained after the other in a state machine fashion. The result was horrid. That's exactly what you'll end up with here. Threads have their place, and this is one of them. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 00/11] mmc: use nonblock mmc requests to minimize latency
On Mon, Jun 27, 2011 at 01:34:48PM +0300, saeed bishara wrote: Russell, I'm curious about the correctness of this patch for systems with outer cache. shouldn't the dsb be issued before the outer cache maintenance? Maybe we should do two passes over SG lists then - one for the inner and another for the outer cache? In effect we could do three passes: 1. Calculate the total size of the SG list to determine whether full cache flush is more efficient. 2. Flush inner cache Then dsb() 3. Flush outer cache Another dsb() ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 00/11] mmc: use nonblock mmc requests to minimize latency
On Tue, Jun 21, 2011 at 11:26:27AM +0200, Per Forlin wrote: Here are the results. It looks like this patch is either a no-op or slightly worse. As people have been telling me that dsb is rather expensive, and this patch results in less dsbs, I'm finding these results hard to believe. It seems to be saying that dsb is an effective no-op on your platform. So either people are wrong about dsb being expensive, the patch is wrong, or there's something wrong with these results/test method. You do have an error in the ported patch, as that hasn't updated the v7 cache cleaning code to remove the dsb() there, but that would only affect the write tests. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 00/11] mmc: use nonblock mmc requests to minimize latency
On Sun, Jun 19, 2011 at 11:17:26PM +0200, Per Forlin wrote: How significant is the cache maintenance over head? Per, Can you measure how much difference this has before and after your patch set please? This moves the dsb() out of the individual cache maintanence functions, such that we will only perform one dsb() per dma_*_sg call rather than one per SG entry. Thanks. arch/arm/include/asm/dma-mapping.h | 11 +++ arch/arm/mm/cache-fa.S |6 -- arch/arm/mm/cache-v4wb.S |2 -- arch/arm/mm/cache-v6.S |6 -- arch/arm/mm/cache-v7.S |3 --- arch/arm/mm/dma-mapping.c |8 6 files changed, 19 insertions(+), 17 deletions(-) diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index 4fff837..853eba5 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -115,6 +115,11 @@ static inline void __dma_page_dev_to_cpu(struct page *page, unsigned long off, ___dma_page_dev_to_cpu(page, off, size, dir); } +static inline void __dma_sync(void) +{ + dsb(); +} + /* * Return whether the given device DMA address mask can be supported * properly. For example, if your device can only drive the low 24-bits @@ -378,6 +383,7 @@ static inline dma_addr_t dma_map_single(struct device *dev, void *cpu_addr, BUG_ON(!valid_dma_direction(dir)); addr = __dma_map_single(dev, cpu_addr, size, dir); + __dma_sync(); debug_dma_map_page(dev, virt_to_page(cpu_addr), (unsigned long)cpu_addr ~PAGE_MASK, size, dir, addr, true); @@ -407,6 +413,7 @@ static inline dma_addr_t dma_map_page(struct device *dev, struct page *page, BUG_ON(!valid_dma_direction(dir)); addr = __dma_map_page(dev, page, offset, size, dir); + __dma_sync(); debug_dma_map_page(dev, page, offset, size, dir, addr, false); return addr; @@ -431,6 +438,7 @@ static inline void dma_unmap_single(struct device *dev, dma_addr_t handle, { debug_dma_unmap_page(dev, handle, size, dir, true); __dma_unmap_single(dev, handle, size, dir); + __dma_sync(); } /** @@ -452,6 +460,7 @@ static inline void dma_unmap_page(struct device *dev, dma_addr_t handle, { debug_dma_unmap_page(dev, handle, size, dir, false); __dma_unmap_page(dev, handle, size, dir); + __dma_sync(); } /** @@ -484,6 +493,7 @@ static inline void dma_sync_single_range_for_cpu(struct device *dev, return; __dma_single_dev_to_cpu(dma_to_virt(dev, handle) + offset, size, dir); + __dma_sync(); } static inline void dma_sync_single_range_for_device(struct device *dev, @@ -498,6 +508,7 @@ static inline void dma_sync_single_range_for_device(struct device *dev, return; __dma_single_cpu_to_dev(dma_to_virt(dev, handle) + offset, size, dir); + __dma_sync(); } static inline void dma_sync_single_for_cpu(struct device *dev, diff --git a/arch/arm/mm/cache-fa.S b/arch/arm/mm/cache-fa.S index 1fa6f71..6eeb734 100644 --- a/arch/arm/mm/cache-fa.S +++ b/arch/arm/mm/cache-fa.S @@ -179,8 +179,6 @@ fa_dma_inv_range: add r0, r0, #CACHE_DLINESIZE cmp r0, r1 blo 1b - mov r0, #0 - mcr p15, 0, r0, c7, c10, 4 @ drain write buffer mov pc, lr /* @@ -197,8 +195,6 @@ fa_dma_clean_range: add r0, r0, #CACHE_DLINESIZE cmp r0, r1 blo 1b - mov r0, #0 - mcr p15, 0, r0, c7, c10, 4 @ drain write buffer mov pc, lr /* @@ -212,8 +208,6 @@ ENTRY(fa_dma_flush_range) add r0, r0, #CACHE_DLINESIZE cmp r0, r1 blo 1b - mov r0, #0 - mcr p15, 0, r0, c7, c10, 4 @ drain write buffer mov pc, lr /* diff --git a/arch/arm/mm/cache-v4wb.S b/arch/arm/mm/cache-v4wb.S index f40c696..523c0cb 100644 --- a/arch/arm/mm/cache-v4wb.S +++ b/arch/arm/mm/cache-v4wb.S @@ -194,7 +194,6 @@ v4wb_dma_inv_range: add r0, r0, #CACHE_DLINESIZE cmp r0, r1 blo 1b - mcr p15, 0, r0, c7, c10, 4 @ drain write buffer mov pc, lr /* @@ -211,7 +210,6 @@ v4wb_dma_clean_range: add r0, r0, #CACHE_DLINESIZE cmp r0, r1 blo 1b - mcr p15, 0, r0, c7, c10, 4 @ drain write buffer mov pc, lr /* diff --git a/arch/arm/mm/cache-v6.S b/arch/arm/mm/cache-v6.S index 73b4a8b..7a842dd 100644 --- a/arch/arm/mm/cache-v6.S +++ b/arch/arm/mm/cache-v6.S @@ -239,8 +239,6 @@ v6_dma_inv_range: strlo r2, [r0]@ write for ownership #endif blo 1b - mov r0, #0 - mcr p15, 0, r0, c7, c10, 4 @ drain write buffer mov pc, lr /* @@ -262,8 +260,6 @@
Re: [RFC] Add Arm cpu topology definition
On Thu, Jun 16, 2011 at 10:49:13AM +0200, Vincent Guittot wrote: @@ -219,6 +219,24 @@ source kernel/Kconfig.freezer menu System Type +config SCHED_MC + bool Multi-core scheduler support + depends on SMP ARM_CPU_TOPOLOGY + default n + help + Multi-core scheduler support improves the CPU scheduler's decision + making when dealing with multi-core CPU chips at a cost of slightly + increased overhead in some places. If unsure say N here. + +config SCHED_SMT + bool SMT scheduler support + depends on SMP ARM_CPU_TOPOLOGY + default n + help + Improves the CPU scheduler's decision making when dealing with + MultiThreading at a cost of slightly increased overhead in some + places. If unsure say N here. Why place these in the system type menu? Wouldn't it be better to place them along side ARM_CPU_TOPOLOGY, and place that along side the SMP option too? + config MMU bool MMU-based Paged Memory Management Support default y @@ -1062,6 +1080,14 @@ if !MMU source arch/arm/Kconfig-nommu endif +config ARM_CPU_TOPOLOGY + bool Support cpu topology definition + depends on SMP CPU_V7 + help + Support Arm cpu topology definition. The MPIDR register defines + affinity between processors which is used to set the cpu + topology of an Arm System. Is there a reason you'd want to disable this? Please also note that it's ARM not Arm. diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h index accbd7c..cb90d0a 100644 --- a/arch/arm/include/asm/topology.h +++ b/arch/arm/include/asm/topology.h @@ -1,6 +1,39 @@ #ifndef _ASM_ARM_TOPOLOGY_H #define _ASM_ARM_TOPOLOGY_H +#ifdef CONFIG_ARM_CPU_TOPOLOGY + +#include linux/cpumask.h + +struct cputopo_arm { + int thread_id; + int core_id; + int socket_id; + cpumask_t thread_sibling; + cpumask_t core_sibling; +}; + +extern struct cputopo_arm cpu_topology[NR_CPUS]; + +#define topology_physical_package_id(cpu)(cpu_topology[cpu].socket_id) +#define topology_core_id(cpu)(cpu_topology[cpu].core_id) +#define topology_core_cpumask(cpu) ((cpu_topology[cpu].core_sibling)) +#define topology_thread_cpumask(cpu) ((cpu_topology[cpu].thread_sibling)) The thing which naggs me about this is that its a static array, and we know from x86 that static arrays of per-cpu data have various issues (cache line bouncing, as well as limitations when we end up with large numbers of CPUs.) Lastly, x86 seems to do this: #define arch_provides_topology_pointers yes and the only effect I can find of that define is in drivers/base/topology.c: #ifdef arch_provides_topology_pointers ... unsigned int cpu = dev-id; \ return show_cpumap(0, topology_##name(cpu), buf); \ ... #else ... return show_cpumap(0, topology_##name(dev-id), buf); \ ... #endif dev-id is type 'u32' which devolves to 'unsigned int' on all supported arches. So it looks to me like this arch_provides_topology_pointers define is completely pointless and we just have useless obfuscation for the hell of it. That's not a criticism of your patch, it's pointing out a difference between x86 and our implementation. +#include linux/cpu.h +#include linux/cpumask.h +#include linux/init.h +#include linux/percpu.h +#include linux/node.h +#include linux/nodemask.h +#include linux/sched.h + +#include asm/cacheflush.h +#include asm/topology.h + +#define hard_smp_mpidr() \ + ({ \ + unsigned int cpunum; \ + __asm__(mrc p15, 0, %0, c0, c0, 5 \ + : =r (cpunum)); \ + cpunum; \ + }) Please add a definition for CPUID_MPIDR to arch/arm/include/asm/cputype.h and a read_cpuid_mpidr() function, and use that instead. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 0/5] Basic ARM devicetree support
On Wed, May 11, 2011 at 10:44:49PM +0200, Grant Likely wrote: Right now it merges cleanly with linux-next and the resulting tree builds and boots at least on qemu. Unless you really object, I'm going to ask Stephen to add the following branch to the /end/ of the list of trees for linux-next so it can easily be dropped it if it causes any problems. As far as the set of five patches looks fine to me, I don't have any objections against them. So I think we can merge them for .40. What I've always worried about is the platform stuff, and that's something I'm going to continue worrying about because I don't think we have sufficient review capacity to ensure that we don't end up with lots of stupidities. Eg, we need to properly sort out how we're going to represent stuff so we don't end up with X IRQ controllers, Y clock events, etc. In other words, I don't want to see DT growing an AT91 IRQ controller, PXA IRQ controller, SA1100 IRQ controller, etc. One of the things we must deal with is how do we reduce the amount of IRQ controller code, clock event code, etc that we have in the kernel tree. That means coming up with some generic representation of these facilities and having the right DT properties in place to be able to describe to the generic representation what's required of it. If we can't do that, then DT isn't solving the problem which Linus has complained about, and we will still be in the situation where platform X wants its own IRQ controller, clock event, etc code. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Booting Integrator/AP with ARM920T
On Mon, Mar 21, 2011 at 11:17:19AM +0100, Linus Walleij wrote: Does anyone beside me *ever* try to actually boot a recent kernel on this thing? How do you do it? Do I need patched U-boots etc, or can I just load a Z-record image of vmlinux into RAM over the serial console? Needless to say, ARM Ltd. has deleted the web pages for official support of this thing as far as I can see. I was going to try out a few patches affecting mach-integrator... That's going back quite a few years... and yes, it wasn't trivial. How it used to be done was with a boot loader called 'milo' which read the kernel out of flash - you had to program it and the kernel using 'afu' via the debugger. Looking at the various scripts I have around, the other way I did it was using Angel (and presumably armsd). I have this .angelrc file: device /dev/ttyS3 options 9600 8N1 baud 115200 base 0x8000 entry 0x8000 #otherfile ramdisk_img.gz #otherbase 0xc080 r0 0x00 r1 21 exec .start-minicom and .start-minicom is: #!/bin/sh exec ~/bin/start-minicom int Not sure if any of this helps, but might provide some hints. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Yet another memory provider: can linaro organize a meeting?
On Thu, Mar 10, 2011 at 03:14:11PM +0100, Marek Szyprowski wrote: Hello, On Tuesday, March 08, 2011 9:14 AM Hans Verkuil wrote: We had a discussion yesterday regarding ways in which linaro can assist V4L2 development. One topic was that of sorting out memory providers like GEM and HWMEM. Today I learned of yet another one: UMP from ARM. http://blogs.arm.com/multimedia/249-making-the-mali-gpu-device-driver-open- source/page__cid__133__show__newcomment/ I really wonder what's the opinion of ARM Linux maintainer on this memory allocator. Russell - could you comment on it? First I've heard about it. I'll have to do some reading first, but I'm rather busy at the present time. As far as DMA memory allocation goes, I do have that patch laying around which preallocates the DMA coherent and writecombine memory, but inspite of sending it to the mailing list, there was very little in the way of feedback. Someone was going to go through the various platforms and work out which could be reduced down to 1MB coherent/1MB writecombine, but I never saw any follow-up to that. I've been debating about just throwing it in the kernel for this coming merge window anyway - I suspect most people just don't care how DMA memory is provided, so long as it works and works reliably. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC] ARM: dma-mapping: outer cache is invalidated twice
On Tue, Feb 15, 2011 at 02:14:55PM +0100, Per Forlin wrote: outer_inv_range () is called twice for DMA_FROM_DEVICE. The first time to get rid of potential writebacks and the second time to get rid of any stale speculative prefetches Correct. outer_inv_range() is a rather expensive operation. In the first case isn't it enough to just call cache_sync()? No. If the CPU speculatively fetches data from the DMA buffer after it's been mapped for DMA, it will bring data into the L2 cache. This data may or may not be up to date with the DMA buffer contents once DMA has completed. As there is no way to know, we have to invalidate the L2 cache (and the L1 cache) after the DMA has completed to avoid any possibility of data corruption. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC] ARM: dma-mapping: outer cache is invalidated twice
On Tue, Feb 15, 2011 at 02:54:21PM +0100, Per Forlin wrote: I don't fully understand this yet. I think you are right but I need a little help to get there myself. I agree, the cache (L1 and L2) must be invalidated after the DMA has completed. Before starting the DMA the write buffers must be drained (cache_sync). Why invalidate the cache before starting the DMA? Think about what happens if you have dirty cache lines in the DMA region. These can be evicted when other cache lines are loaded, which will result in them overwriting contents of memory. If the DMA device has already written to that memory, the result is data corruption. So, the invalidate prior to DMA is to get rid of any dirty cache lines which could be written back to memory. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] arm: Improve MMC performance on Versatile Express
On Tue, Feb 01, 2011 at 01:34:59PM +, Pawel Moll wrote: And to prove the point, I have MMCI running at up to 4Mbps, an 8 fold increase over what the current fixed upper-rate implementation does. The adaptive rate implementation is just a proof of concept at the moment and requires further work to improve the rate selection algorithm. Great, I've terribly glad you managed to have a go at this (I honestly wanted to, but simply had no time). I'm looking forward to see the patches and will be more than happy to backport them for the sake of the Linaro guys using 2.6.35 and 2.6.37 right now. On our side we did extend the FIFO and performed some tests (not very extensive yet though). The change seems not to break anything and help in the pathological (heavy USB traffic) scenario. When I get your changes and some official FPGA release, I'll try to push the bandwidth limits even further - hopefully changes will complement. You can't push it any further without increasing the CPU/bus clock rates. My measurements show that it takes the CPU in the region of 6-9us to unload 32 bytes from the FIFO, which gives a theoretical limit of 2.8 to 4.2Mbps, depending on how the platform booted (some reboots its consistently in the order of 6us, some boots its consistently around 9us.) The real solution to this is for there to be proper working DMA support implemented on ARM platforms, In case of VE this is all about getting an engine into the test chips, what didn't happen for A9 (the request lines are routed between the motherboard and the tile and IO FPGA can - theoretically - use the MMCI requests). As far as I'm told this cell is simply huge (silicon-wise) and therefore it's the first candidate to cut down when area is scarce... Anyway, I've spoken to guys around and asked them to keep the problem in mind, so we may get something with the next releases. Bear in mind that PL18x + PL08x doesn't work. Catalin forwarded my concerns over this to ARM Support - where I basically ask how to program the hardware up to DMA a single 64K transfer off a MMC card into a set of scattered memory locations. I've yet to have a response, so I'll take it that it's just possible (the TRMs say as much). The problem is that for a transfer, the MMCI produces BREQ * n + LBREQ, and the DMAC will only listen for a LBREQ if it's in peripheral flow control. If it's in peripheral flow control, then it ignores the transfer length field in the control register, only moving to the next LLI when it sees LBREQ or LSREQ. It ignores LBREQ and LSREQ in DMAC flow control mode.. You can DMA almost all the data too/from the MMCI, but you miss the last half-fifo-size worth of data. While you can unload that manually for a read, you can't load it manually for a write. With peripheral flow control, you can only DMA the requested data to a single contiguous buffer without breaking the MMC request into much smaller chunks. As Peter Pearse's PL08x code seems to suggest, the maximum size of those chunks is 1K. This seems to be a fundamental problem with the way each primecell has been designed. So, I do hope that someone decides to implement something more reasonable if Versatile Express were to get a DMA controller. If it's another PL08x then it isn't worth it - it won't work. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] arm: Improve MMC performance on Versatile Express
On Mon, Jan 24, 2011 at 12:27:16PM -, Pawel Moll wrote: So - we'll try to enlarge FIFO. For the moment - playing with interrupts affinity seem to be a viable workaround. I don't think enlarging the FIFO will help too much. The issue is whether the CPU can keep up with the data rate coming off the card. If it can't, then no matter how large the FIFO is, it will eventually overflow. The real answer is to avoid PIO mode, and use DMA support. However, I've had problems using DMA on the ARM development boards. You can find details my DMA issues internally within ARM by talking to Catalin. The alternative answer, I believe implemented by some of ARMs silicon partners, is to turn the card clock off when the FIFO becomes full/empty to stop it sending more data. I think this violates some of the MMC/SD requirements, but it seems to work for the silicon partners just fine. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] arm: Improve MMC performance on Versatile Express
On Mon, Jan 24, 2011 at 04:13:14PM +, Pawel Moll wrote: I don't think enlarging the FIFO will help too much. The issue is whether the CPU can keep up with the data rate coming off the card. If it can't, then no matter how large the FIFO is, it will eventually overflow. Yes, I realize that. But so far the only time when problem happens is the timeout caused by ISP1761 handler. That's fixable by having the block driver retry. I have patches lodged with cjb which rework the MMC block driver error handling, and I now have MMC rootfs entirely usable with the ISP1761 driver. With ISP1761 also fixed (god knows why the driver maintainer isn't doing anything with that patch) then the problem goes away. So really this is a non-issue. If we substantially increase the FIFO depth, we'll just have much more margin - enough to mostly work. To my taste, the 1.3ms required to service the USB interrupt is already waaay to long. I'd consider any substantially larger latencies pathological, so making sure that we have margin of 3, maybe even 5 ms sounds good enough to me. Well, fixing MMCI because ISP1761 is buggy is not the way forward. The answer is to fix the broken ISP1761 driver. We know as it currently stands in mainline that it's utterly unusable with certain serial devices. That doesn't mean we go off and fix MMCI. The real answer is to avoid PIO mode, and use DMA support. However, I've had problems using DMA on the ARM development boards. You can Well, using DMA won't be easy on VE, will it? ;-) Besides even with this, in some extreme situation, the bus could be potentially stalled long enough to cause an underrun. Yes, very unlikely, but not impossible. I'm running SD rootfs alongside NFS/ssh/NTP and the buggy ISP1761. With the pile of 'fixes' patches I have here (which are currently stuck with various maintainers) I have a completely stable system. I've also had it running gstreamer on CLCD with AACI too, again with SD rootfs. Provided the video is already loaded off the SD card (because the transfer rate is too slow) it's fine. The only remaining problem I have with it is !£$%% busybox shell which doesn't like readonly rootfs with command history. The last command is /sbin/reboot - I'm sure you can imagine what keeps on happening, particularly at the most annoying points in time. The alternative answer, I believe implemented by some of ARMs silicon partners, is to turn the card clock off when the FIFO becomes full/empty to stop it sending more data. I think this violates some of the MMC/SD requirements, but it seems to work for the silicon partners just fine. Oh no, that's absolutely legal - see JEDEC 84-A441, p. 7.7: The MultiMediaCard bus clock signal can be used by the host to put the card into energy saving mode, or to control the data flow (to avoid under-run or over-run conditions) on the bus. So this would be the technically correct fix. The problem is that this would require even more MMCI modifications then enlarging FIFO, so it's unlikely to happen. Well as I see it, it's pointless enlarging the FIFO. ARM Ltd isn't going to give me updated FPGA images for the Integrator/CP, Versatile PB926, Realview EB and Versatile Express. Given that the problem is already fixed via a set of patches, I see no reason to mess about with the hardware, thereby making the driver more complicated for *no* benefit. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] arm: Improve MMC performance on Versatile Express
On Mon, Jan 24, 2011 at 04:24:00PM +, Russell King - ARM Linux wrote: On Mon, Jan 24, 2011 at 04:13:14PM +, Pawel Moll wrote: The real answer is to avoid PIO mode, and use DMA support. However, I've had problems using DMA on the ARM development boards. You can Well, using DMA won't be easy on VE, will it? ;-) Besides even with this, in some extreme situation, the bus could be potentially stalled long enough to cause an underrun. Yes, very unlikely, but not impossible. I'm running SD rootfs alongside NFS/ssh/NTP and the buggy ISP1761. With the pile of 'fixes' patches I have here (which are currently stuck with various maintainers) I have a completely stable system. I've also had it running gstreamer on CLCD with AACI too, again with SD rootfs. Provided the video is already loaded off the SD card (because the transfer rate is too slow) it's fine. The only remaining problem I have with it is !£$%% busybox shell which doesn't like readonly rootfs with command history. The last command is /sbin/reboot - I'm sure you can imagine what keeps on happening, particularly at the most annoying points in time. I should also point out that my Versatile Express has been on continuously for the last two to three weeks, running off SD rootfs, and since fixing the ISP1761 driver and MMC block driver error handling there's been zero issues with MMC or ISP1761. So, MMCI overruns is now a complete non-issue for me. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] arm: Improve MMC performance on Versatile Express
On Mon, Jan 24, 2011 at 04:39:21PM -, Pawel Moll wrote: Well, fixing MMCI because ISP1761 is buggy is not the way forward. The answer is to fix the broken ISP1761 driver. Totally agree. The thing is that you are the only person right now who doesn't have any problems. That's not my problem. A simple search of the linux-usb mailing list will give you the patch: http://marc.info/?l=linux-usbm=129469114904445w=2 The others can quickly benefit from the workaround. Others _can_ quickly benefit from the ISP1761 fix by applying the above patch, which not only fixes the MMCI overruns, but also allows USB to serial devices to work with Versatile Express. The above patch is the correct solution. For christ sake, that patch is a fix for a *known* errata issued by the ISP1761 manufacturer. Well as I see it, it's pointless enlarging the FIFO. ARM Ltd isn't going to give me updated FPGA images for the Integrator/CP, Versatile PB926, Realview EB and Versatile Express. If we succeed with this, you (and all other users) will get the improved FPGA image. But for VE only indeed. That's utterly useless and completely pointless. Look, stop jumping off a cliff with this and come back to reality. The ISP1761 is buggy. We have a fix. We have a tested fix. We have a tested fix out on mailing lists for everyone to use. It fixes more than MMC overruns and makes the ISP1761 usable. The only problem is that it's taking *FAR* too long for it to get into mainline for no apparant reason. That doesn't stop anyone taking the tested patch in the URL above and gaining proper USB functionality. The right thing to do is to follow up on my reply on the ISP1761 thread to find out why it's taking soo long for it to get into Linus' tree. I repeat, with the proper set of patches in mainline THERE IS NO ISSUE WITH MMCI. THE HARDWARE DOES NOT NEED FIXING. Please listen to me. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] arm: Improve MMC performance on Versatile Express
My final mail on this subject. I'm adding Philippe and Catalin so that they're in the loop on this. Take a mainline kernel. Apply the attached three patches. MMC will then work without any problems. No hacks required. There is *absolutely* *no* need to waste time with hardware modifications. With this you will find that MMCI FIFO underruns/overruns are no longer any problem. diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index bfc8a8a..264a6bb 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -245,6 +245,20 @@ static u32 mmc_sd_num_wr_blocks(struct mmc_card *card) return result; } +static void send_stop(struct mmc_card *card, struct request *req) +{ + struct mmc_command cmd; + int err; + + memset(cmd, 0, sizeof(struct mmc_command)); + cmd.opcode = MMC_STOP_TRANSMISSION; + cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC; + err = mmc_wait_for_cmd(card-host, cmd, 0); + if (err) + pr_err(%s: error %d sending stop command\n, + req-rq_disk-disk_name, err); +} + static u32 get_card_status(struct mmc_card *card, struct request *req) { struct mmc_command cmd; @@ -255,9 +269,9 @@ static u32 get_card_status(struct mmc_card *card, struct request *req) if (!mmc_host_is_spi(card-host)) cmd.arg = card-rca 16; cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC; - err = mmc_wait_for_cmd(card-host, cmd, 0); + err = mmc_wait_for_cmd(card-host, cmd, 2); if (err) - printk(KERN_ERR %s: error %d sending status command, + pr_err(%s: error %d sending status command\n, req-rq_disk-disk_name, err); return cmd.resp[0]; } @@ -336,7 +350,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req) struct mmc_blk_data *md = mq-data; struct mmc_card *card = md-queue.card; struct mmc_blk_request brq; - int ret = 1, disable_multi = 0; + int ret = 1, disable_multi = 0, retry = 0; mmc_claim_host(card-host); @@ -432,6 +446,53 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req) * programming mode even when things go wrong. */ if (brq.cmd.error || brq.data.error || brq.stop.error) { + status = get_card_status(card, req); + + /* First print what's up */ + if (brq.cmd.error) + pr_err(%s: error %d sending read/write command, card status %#x\n, + req-rq_disk-disk_name, brq.cmd.error, + status); + + if (brq.data.error) + pr_err(%s: error %d transferring data, sector %u, nr %u, cmd response %#x, card status %#x\n, + req-rq_disk-disk_name, brq.data.error, + (unsigned)blk_rq_pos(req), + (unsigned)blk_rq_sectors(req), + brq.cmd.resp[0], status); + + if (brq.stop.error) + pr_err(%s: error %d sending stop command, original cmd response %#x, card status %#x\n, + req-rq_disk-disk_name, brq.stop.error, + brq.cmd.resp[0], status); + + /* +* Now check the current card state. If it is +* in some data transfer mode, tell it to stop +* (and hopefully transition back to TRAN.) +*/ + if (R1_CURRENT_STATE(status) == R1_STATE_DATA || + R1_CURRENT_STATE(status) == R1_STATE_RCV) + send_stop(card, req); + + /* +* r/w cmd failure - get_card_status() should +* tell us why the command was not accepted +*/ + if (brq.cmd.error retry 2) { + /* +* if it was a r/w cmd crc error, or illegal +* command (eg, issued in wrong state) then +* retry - we should have corrected the +* state problem above. +*/ + if (status (R1_COM_CRC_ERROR | + R1_ILLEGAL_COMMAND)) { + retry++; + continue; + } + } + if (brq.data.blocks 1 rq_data_dir(req) == READ) {
Re: [PATCH] arm: Improve MMC performance on Versatile Express
On Mon, Jan 24, 2011 at 07:59:03PM +, Pawel Moll wrote: If you're flooding the system with USB traffic, enlargening the FIFO size won't help. Making the FIFO larger just decreases the _interrupt_ _latency_ requirements. It doesn't mean you can cope with the amount of data being transferred. On VE both ISP and MMCI are sharing the same static memory interface, What has that to do with it? If the static memory controller was the bottleneck, don't you think that two CPUs running in parallel, one reading data from the ISP1761 and the other reading the MMCI would suffer bus starvation? Your HACK HACK HACK patch shows that clearly isn't the case. You've already told me that you've measured the ISP1761 interrupt handler taking 1.3ms to empty data off of the chip. If that's 60K of data, that's a data rate of around 47MiB/s. At 521kHz transfer rate, it takes about 490us for MMCI to half-fill its FIFO, or 980us to fully fill it. It takes (measured) about 6-9us to unload 32 bytes of data from the FIFO. Translating the CPU read rate, that's a data rate of around 4MiB/s. So I put it to you that there's plenty of bus bandwidth present to service both the ISP1761 and MMCI. What we're lacking is CPU bandwidth. I guess you haven't thought about moving MMCI to an adaptive clocking solution? What I'm suggesting is halve the clock rate on FIFO error and retry. Increase the clock rate on each successful transfer up to the maximum provided by the core MMC code. That should _significantly_ increase the achievable PIO data rate way beyond what a deeper FIFO could ever hope to achieve, and will allow it to adapt to situations where you load the system up beyond the maximum latency which the MMCI can stand. This would benefit a whole range of platforms, improving performance across the board, which as you've already indicated merely going for a deeper FIFO would be unable to do. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] arm: Improve MMC performance on Versatile Express
On Fri, Jan 21, 2011 at 02:59:35PM -0500, Nicolas Pitre wrote: On Fri, 21 Jan 2011, Pawel Moll wrote: VE suffers from serious problem with MMC transfers - low performance, errors when other IO peripherals (especially USB) are used at the same time etc. It all boils down to the MMC controller short FIFO and - in result - timing constrains. The most problematic case - USB driver hogging CPU and MMC FIFO under/overruns in the result - can be mitigated on SMP system by distributing interrupts handling for these peripherals between cores. Wouldn't the ultimate solution be to simply use FIQs to service the MMC FIFO? Could you suggest how to route an arbitary interrupt to the FIQ using the GIC? On systems which implement security extensions, the non-secure world can only use IRQs and not FIQs. The secure world can use FIQs - in which case interrupts marked as secure interrupts can go to FIQ. On systems without security extensions, the GIC only supports IRQ, and you need a second GIC implemented for FIQs. I'm not aware of any system doing that. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: RFC: Dynamic hwcaps
On Tue, Dec 07, 2010 at 10:45:42AM +, Dave Martin wrote: Yes-- though I didn't elaborate on it. You need a packager that can understand, say, that a binary built for ARMv5 EABI can interoperate with ARMv7 binaries etc. Again, I've heard it suggested that RPM can handle this, but I haven't looked at it in detail myself. That is indeed the case - as on x86, it used to be common to build the majority of the distribution for i386, and glibc and a few other bits for a range of ix86 CPUs. rpm and yum know that i386 is compatible with i486, which is compatible with i586 etc, so it will install an i386 package on i686 if no i486, i586 or i686 package is available. It does the same for ARM with ARMv3, ARMv4 etc. The dynamic hwcaps approach doesn't really solve that problem: Has anyone investigated whether it is possible to power down things like Neon etc meanwhile leaving the rest of the CPU running? I've not seen anything in the ARM documentation to suggest that's the case. Even in MPCore based systems, the interface between the SCU and individual processors by default doesn't have the necessary clamps built in to allow individual CPUs to be powered off, and I'm not aware of any designs which decided to enable this feature (as there's a performance penalty). So I'd be really surprised if there was any support for powering down Neon separately from the host CPU. If that's the case, it's entirely pointless discussing what userspace can or can't do - if you have Neon available and can't power it down, and it's faster for doing something, you might as well use it so you can put the main CPU into WFI mode or get on with some other useful work. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: RFC: Dynamic hwcaps
On Tue, Dec 07, 2010 at 03:06:51PM +, Dave Martin wrote: Hi, On Tue, Dec 7, 2010 at 11:04 AM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Tue, Dec 07, 2010 at 10:45:42AM +, Dave Martin wrote: Yes-- though I didn't elaborate on it. You need a packager that can understand, say, that a binary built for ARMv5 EABI can interoperate with ARMv7 binaries etc. Again, I've heard it suggested that RPM can handle this, but I haven't looked at it in detail myself. That is indeed the case - as on x86, it used to be common to build the majority of the distribution for i386, and glibc and a few other bits for a range of ix86 CPUs. rpm and yum know that i386 is compatible with i486, which is compatible with i586 etc, so it will install an i386 package on i686 if no i486, i586 or i686 package is available. It does the same for ARM with ARMv3, ARMv4 etc. That sounds plausible. That sounds like doubt. I've used rpm extensively over the last 10 years or so, both on x86 and ARM. I've built many versions of Red Hat and Fedora for ARM. My ARM machines here (including the one which is going to send this email) run the result of that - and is currently a mixture of ARMv3 and ARMv4 Fedora packages. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev