* [PATCH] ARM: OMAP: resolve sparse warning concerning debug_card_init()
From: Paul Walmsley @ 2012-10-09 20:29 UTC (permalink / raw)
To: linux-arm-kernel
Commit 801475ccb2b2c1928b22aec4b9e5285d9e347602 ("ARM: OMAP: move
debug_card_init() function") results in the following new sparse
warning:
arch/arm/plat-omap/debug-devices.c:71:12: warning: symbol 'debug_card_init' was not declared. Should it be static?
Fix by implementing Tony's suggestion to add a "sideways include" to the
new location of the debug-devices.h file in arch/arm/mach-omap2/.
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Igor Grinberg <grinberg@compulab.co.il>
Cc: Tony Lindgren <tony@atomide.com>
---
arch/arm/plat-omap/debug-devices.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/plat-omap/debug-devices.c b/arch/arm/plat-omap/debug-devices.c
index c7a4c09..5a4678e 100644
--- a/arch/arm/plat-omap/debug-devices.c
+++ b/arch/arm/plat-omap/debug-devices.c
@@ -16,6 +16,7 @@
#include <linux/smc91x.h>
#include <mach/hardware.h>
+#include "../mach-omap2/debug-devices.h"
/* Many OMAP development platforms reuse the same "debug board"; these
* platforms include H2, H3, H4, and Perseus2.
--
1.7.10.4
^ permalink raw reply related
* ARM: hw_breakpoint: silent EPERM when setting ARM_DSCR_MDBGEN on ARM_DEBUG_ARCH_V7_ECP14
From: Will Deacon @ 2012-10-09 21:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAPfQmKJAvd2R8Je-uRdKkkhgQRodZFxP1N_QzbjKm8q7q1HgaQ@mail.gmail.com>
On Tue, Oct 09, 2012 at 06:05:53PM +0100, Valentin Pistol wrote:
> Will,
Hi Valentin,
> On Tue, Sep 18, 2012 at 3:04 AM, Will Deacon <will.deacon@arm.com> wrote:
> >
> > On Fri, Sep 14, 2012 at 09:08:24AM +0100, Valentin Pistol wrote:
> > > Is there a reliable way to determine that they are locked down?
> > > Any particular register/bit I can check to confirm?
> >
> > You can take a look at the DBGAUTHSTATUS register and try to determine the
> > signal values for SPNIDEN, DBGEN and NIDEN.
>
> DBGAUTHSTATUS=0xaa confirming DBGEN is LOW. Since then I found out
> that there are two types of OMAP devices:
> GP (General Purpose) and HS (High Security) and supposedly Debug
> Capabilities are only available on GP devices.
> Thus, I ordered a Pandaboard ES with OMAP4460 and tested it with a
> prebuilt Ubuntu 12.04 image.
> After boot monitor mode is still not set, the DSCR reads the same
> 0x01030002 just as with the previous Galaxy Nexus.
Can you try this with my hw-breakpoint branch please? I've reworked a fair
amount of code there, so it might at least be more verbose if it fails.
> I contacted TI and they seem to say hw breakpoints/watchpoints are
> available on Pandaboard but they are confused about monitor mode,
> saying it's not available on Pandaboard GP.
> See http://e2e.ti.com/support/omap/f/849/p/216276/770995.aspx for the
> discussion.
In that discussion, the TI folks are confusing `Monitor-mode Debug' with
`Secure Monitor Mode'. They are two completely different things and (for the
purposes of this discussion), unrelated. You should press them further on
this!
> Have you used a Pandaboard and can comment on how to enable the
> breakpoints/watchpoints?
I haven't tried it, but I do have a Panda now so I could give it a go when I
get a chance. I suspect there's something related to power-management at
play... for JTAG, you need to hack MLO as described here:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka15314.html
maybe something similar is required for self-hosted (monitor mode) debug?
> Could you also mention a specific development board that you are using
> and recommend for access to these features?
I use the ARM Versatile Express platform, but I think it's probably quite
expensive to get hold of if you don't work for ARM.
Will
^ permalink raw reply
* [GIT PULL] Disintegrate UAPI for arm64 [ver #2]
From: Catalin Marinas @ 2012-10-09 21:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <6973.1349811059@warthog.procyon.org.uk>
On Tue, Oct 09, 2012 at 08:30:59PM +0100, David Howells wrote:
> Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> > It still fails on arm64. The reason is that I had a __SYSCALL_COMPAT
> > guard to provide either the 32-bit syscalls or the 64-bit (generic) ones
> > via asm/unistd.h. With this change:
>
> Hmmm.
>
> Why does asm/unistd.h get #included for the compat bits at all? Looking in
> arch/arm64/kernel/sys32.S could the compat syscall table be generated by a
> direct #inclusion of asm/unistd32.h instead?
Yes, that's what I tried. But since unistd.h gets included indirectly as
well, I have to change the __NR_* definitions in unistd32.h to avoid
conflicts (once done, it's fine again).
> Is there a reason that asm/unistd32.h needs to declare __NR_ macros? Looking
> at signal32.c and sys_compat.c you only need:
>
> __ARM_NR_compat_cacheflush
> __ARM_NR_compat_set_tls
>
> and:
>
> __NR_sigreturn
> __NR_rt_sigreturn
> __NR_restart_syscall
>
> could you define __ARM_NR_ versions of the last three and get rid of all the
> __NR_ constant definitions by putting the numbers directly into the __SYSCALL
> macros?
I would name them __NR_compat_ rather than __ARM_NR_ as the latter
usually means architecture-specific system call.
> > The solution is to either keep the __SYSCALL_COMPAT guard in place or
> > rename all the __NR_* macros in unistd32.h to __NR_compat_* and include
> > unistd32.h explicitly where needed (kernel-only header anyway). Since
> > the arm64 kernel would not export 32-bit headers, I would go with the
> > second solution (tried it already).
>
> That sounds workable too... Certainly easier to do with a text editor than
> folding the compat __NR_ values into their __SYSCALL() macros.
>
> Either way, you can then blithely always include unistd32.h without having to
> wriggle to avoid the duplicate symbols.
>
> > But you need to re-generate the arm64 headers again.
>
> Regeneration is certainly quick and (usually) easy, but I need to change the
> base to end up with a different result. Do you want me to rename all the
> __NR_* to __NR_compat_* before doing that (if you have a patch to do so, I can
> incorporate that on the arm64 branch prior to doing the disintegration).
I'll push a patch tomorrow so you can run your script on that branch. I
can also fix things up after merging your current branch (it breaks
bisectability but it doesn't matter much for arm64, it's only a few
patches).
> > BTW, I see the script generated some pretty much empty
> > uapi/asm/unistd.h. Is it possible to using something like Kbuild and
> > just add "generic-y += ..." to just point it to the
> > include/uapi/asm-generic header?
>
> Yes. Note that I was asked not to lose copyright notices if they were
> present, which is why you've ended up with that.
>
> At this point, I'd rather not adjust the disintegration script, so fixing it
> up after would be simplest.
OK, it makes sense.
Thanks.
--
Catalin
^ permalink raw reply
* [PATCH 0/3] new ARM-soc warning fixes
From: Olof Johansson @ 2012-10-09 21:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201210092001.26947.arnd@arndb.de>
On Tue, Oct 09, 2012 at 08:01:26PM +0000, Arnd Bergmann wrote:
> On Tuesday 09 October 2012, Olof Johansson wrote:
> > On Tue, Oct 09, 2012 at 05:34:18PM +0200, Arnd Bergmann wrote:
> > > Hi Olof,
> > >
> > > Here are three more fixes that I'd like to merge through the arm-soc
> > > as time permits. All of them should be completely harmless and they
> > > only fix harmless gcc warnings.
> > >
> > > I don't know what your plans are for sending the next fixes branch.
> > > I've uploaded these into a new "fixes2" branch on top of the patches
> > > that are already in "fixes". We can either send them all together,
> > > or you send the ones that are already queued first and these ones
> > > with the next batch. Feel free to rebase if necessary.
> >
> > I've pulled it in and will send up with fixes tonight (Tony has OMAP
> > fixes coming, others might too).
> >
>
> Ok, thanks!
>
> I just noticed that I was missing yet another one, which I now added
> directly. It was part of a longer series I sent last week. I wasn't
> really sure whether this one would better be included in arm-soc or
> Russell's arm tree and then I ended up not putting it in either series :(
OK, I applied on top of the rest.
-Olof
^ permalink raw reply
* [GIT PULL] ARM: OMAP: warnings/hwmod/clock fixes for pre-3.7-rc1
From: Tony Lindgren @ 2012-10-09 21:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1210091928180.5736@utopia.booyaka.com>
* Paul Walmsley <paul@pwsan.com> [121009 12:31]:
> On Tue, 9 Oct 2012, Tony Lindgren wrote:
>
> > * Paul Walmsley <paul@pwsan.com> [121009 09:05]:
> > >
> > > N800 isn't booting; this is a problem present in the base commit and
> > > is due to serial driver breakage:
> > >
> > > http://www.spinics.net/lists/arm-kernel/msg196034.html
> >
> > This might be related also to the n800 booting in sys mode
> > that causes issues with the recent hyp patches. A fix for
> > that has been queued, for more info see the thread
> > "v2 2/7] ARM: virt: allow the kernel to be entered in HYP mode".
>
> Thanks for the pointer. That one would break the boot quite early, right?
> This one has the same symptoms as the serial issue, where it bombs out
> after:
>
> [ 0.948394] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
> [ 0.974273] omap_uart.0: ttyO0 at MMIO 0x4806a000 (irq = 88) is a OMAP UART0
> [ 0.986968] omap_uart.1: ttyO1 at MMIO 0x4806c000 (irq = 89) is a OMAP UART1
>
> (hang)
Yes the sys mode hangs right away and only prints a few lines
with earlyprintk.
Regards,
Tony
^ permalink raw reply
* [PATCH] ARM: OMAP: move old debug-devices.c and debug-leds.c to be OMAP2+ only for now
From: Tony Lindgren @ 2012-10-09 21:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1210092008370.5736@utopia.booyaka.com>
* Paul Walmsley <paul@pwsan.com> [121009 13:12]:
>
> OK if you want to leave it there, it's fine with me. Looks like it
> should go into drivers/ at some point though?
Yes it should be just a regular device driver
eventually at some point. But no rush, let's first
get the dependencies between device drivers and core
omap code removed by removing the remaining mach and
plat includes.
Regards,
Tony
^ permalink raw reply
* pm: add suspend_mem and suspend_standby support
From: Rafael J. Wysocki @ 2012-10-09 22:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121009145854.GB14079@kroah.com>
On Tuesday 09 of October 2012 07:58:54 Greg Kroah-Hartman wrote:
> On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 22:02 Sun 07 Oct , Rafael J. Wysocki wrote:
> > > On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 00:18 Sun 07 Oct , Rafael J. Wysocki wrote:
> > > > > On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > Hi,
> > > > > >
> > > > > > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > > > > >
> > > > > > Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > > > > >
> > > > > > are available in the git repository at:
> > > > > >
> > > > > >
> > > > > > git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > > > > >
> > > > > > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > > > > >
> > > > > > arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > > > > >
> > > > > > ----------------------------------------------------------------
> > > > > > pm: add suspend_mem and suspend_standby support
> > > > > >
> > > > > > Today when we go to suspend we can not knwon at drivers level if we go in
> > > > > > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > > > > > suspend_standby.
> > > > >
> > > > > No way. Device drivers shouldn't be concerned about that.
> > > > I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> > > > need special handling when switching to slow_clock
> > >
> > > Well, my answer to that is: please fix your platform code instead of
> > > hacking the PM core to work around its problems.
> > how can I fix drivers pm issue when I no way to known at driver level the
> > real suspend, the PM core is supposed to proivde the right information to the
> > drivers so the driver can put it's in the right pm mode. If the pm core can not
> > provide such inforation the PM core is broken as we will have to do dirty
> > hack.
>
> Why do you need to know the difference in your driver? We used to
> provide this information a long time ago, but it turned out to not be
> needed at all and just caused problems.
We actualy have never made a difference between "standby" and "suspend" as
far as drivers are concerned. What we used to have was one "suspend" callback
for suspend to RAM and all phases of hibernation, so we needed to pass an
argument indicating what operation was to be carried out to it.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* pm: add suspend_mem and suspend_standby support
From: Rafael J. Wysocki @ 2012-10-09 22:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121009151704.GP12801@game.jcrosoft.org>
On Tuesday 09 of October 2012 17:17:04 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 07:58 Tue 09 Oct , Greg Kroah-Hartman wrote:
> > On Tue, Oct 09, 2012 at 01:46:33PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 22:02 Sun 07 Oct , Rafael J. Wysocki wrote:
> > > > On Sunday 07 of October 2012 15:12:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > On 00:18 Sun 07 Oct , Rafael J. Wysocki wrote:
> > > > > > On Saturday 06 of October 2012 18:14:29 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > The following changes since commit 5f3d2f2e1a63679cf1c4a4210f2f1cc2f335bef6:
> > > > > > >
> > > > > > > Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc (2012-10-06 03:16:12 +0900)
> > > > > > >
> > > > > > > are available in the git repository at:
> > > > > > >
> > > > > > >
> > > > > > > git://git.jcrosoft.org/linux-2.6.git tags/pm_suspend_standby_mem
> > > > > > >
> > > > > > > for you to fetch changes up to b73c8f97aa8e720bd3b921159687d00626c99d63:
> > > > > > >
> > > > > > > arm: at91: drop at91_suspend_entering_slow_clock (2012-10-06 18:06:25 +0800)
> > > > > > >
> > > > > > > ----------------------------------------------------------------
> > > > > > > pm: add suspend_mem and suspend_standby support
> > > > > > >
> > > > > > > Today when we go to suspend we can not knwon at drivers level if we go in
> > > > > > > STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> > > > > > > suspend_standby.
> > > > > >
> > > > > > No way. Device drivers shouldn't be concerned about that.
> > > > > I do need it on at91 as we swith to slow_clock in MEM suspend and some ip
> > > > > need special handling when switching to slow_clock
> > > >
> > > > Well, my answer to that is: please fix your platform code instead of
> > > > hacking the PM core to work around its problems.
> > > how can I fix drivers pm issue when I no way to known at driver level the
> > > real suspend, the PM core is supposed to proivde the right information to the
> > > drivers so the driver can put it's in the right pm mode. If the pm core can not
> > > provide such inforation the PM core is broken as we will have to do dirty
> > > hack.
> >
> > Why do you need to know the difference in your driver? We used to
> > provide this information a long time ago, but it turned out to not be
> > needed at all and just caused problems.
> because on at91 I need to handle the mem standby at drviers level.
This is not an answer. You're basically saying "it has to be done this way,
because it has to be done this way".
> We do it today already by a hack in different drivers at91_udc (usb device),
> atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
> to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
> clock - this is done at soc level - but those IP have issue and need specific
> care before doing so. Ohterwise when the SoC will wakeup but those IP will not
>
> in this patch series I send the update of those 3 drivers too
> and kill the hack
Well, let's start over. What's the difference between "suspend to RAM" (mem)
and "standby" on your platform?
> > > Any generic framework is supposed to evolve for real user need here I've one
> > > so I udpate the core to mach a need
Yes, if there are more users needing a change. If there's only one, I think not
really.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* pm: add suspend_mem and suspend_standby support
From: Rafael J. Wysocki @ 2012-10-09 22:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121009155241.GQ12801@game.jcrosoft.org>
On Tuesday 09 of October 2012 17:52:41 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 11:44 Tue 09 Oct , Alan Stern wrote:
> > On Tue, 9 Oct 2012, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >
> > > > Why do you need to know the difference in your driver? We used to
> > > > provide this information a long time ago, but it turned out to not be
> > > > needed at all and just caused problems.
> > > because on at91 I need to handle the mem standby at drviers level.
> > >
> > > We do it today already by a hack in different drivers at91_udc (usb device),
> > > atmel_serail and at91_ohci. Those 3 IP have specifci handling when switching
> > > to mem pm. On at91 when switch to mem we shutdown everything and run form a slow
> > > clock - this is done at soc level - but those IP have issue and need specific
> > > care before doing so. Ohterwise when the SoC will wakeup but those IP will not
> > >
> > > in this patch series I send the update of those 3 drivers too
> > > and kill the hack
> >
> > How about adding a platform-specific callback routine to tell drivers
> > whether or not they will run from the slow clock? Something like:
> >
> > bool at91_suspend_will_use_slow_clock(void);
> >
> > That would not involve making any changes to the driver core, and your
> > three drivers could still get the information they need.
> I already do so and I want to drop it as the IP is shared with multiple ARCH
> and I want to have a clean code no hack
So, what about actually adding a platform layer to your PM instead of doing
all PM in device drivers?
ACPI has exactly the same problem that you have, but it's never has to tell
device drivers what system sleep state is going to be entered. Drivers just
call acpi_pm_device_sleep_state() and that tells them what power state to
put the device into (which depends on the sleep state being entered, but
drivers really don't need to know that state).
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* [PATCH 1/7] ARM: tegra: cpuidle: separate cpuidle driver for different chips
From: Stephen Warren @ 2012-10-09 22:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1349691981-31038-2-git-send-email-josephl@nvidia.com>
On 10/08/2012 04:26 AM, Joseph Lo wrote:
> The different Tegra chips may have different CPU idle states and data.
> Individual CPU idle driver make it more easy to maintain.
> diff --git a/arch/arm/mach-tegra/cpuidle.c b/arch/arm/mach-tegra/cpuidle-tegra20.c
> similarity index 72%
> copy from arch/arm/mach-tegra/cpuidle.c
> copy to arch/arm/mach-tegra/cpuidle-tegra20.c
> index 4e0b07c..e2fc26a 100644
> --- a/arch/arm/mach-tegra/cpuidle.c
> +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c
> @@ -1,24 +1,22 @@
> /*
> - * arch/arm/mach-tegra/cpuidle.c
> + * CPU idle driver for Tegra20 CPUs
> *
> - * CPU idle driver for Tegra CPUs
> - *
> - * Copyright (c) 2010-2012, NVIDIA Corporation.
> + * Copyright (c) 2010-2012, NVIDIA Corporation. All rights reserved.
> * Copyright (c) 2011 Google, Inc.
> * Author: Colin Cross <ccross@android.com>
> * Gary King <gking@nvidia.com>
> *
> - * Rework for 3.3 by Peter De Schrijver <pdeschrijver@nvidia.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> *
> * This program is distributed in the hope that it will be useful, but WITHOUT
> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
While that is the correct (c) header for new work submitted upstream,
given this file is existing, just renamed and tweaked a bit, I don't
believe you want to be changing the (c) header at all. Same for
cpuidle-tegra30.c.
^ permalink raw reply
* [PATCH 0/7] ARM: tegra30: cpuidle: add LP2 support
From: Stephen Warren @ 2012-10-09 22:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1349691981-31038-1-git-send-email-josephl@nvidia.com>
On 10/08/2012 04:26 AM, Joseph Lo wrote:
> The CPU idle LP2 is a power gating idle mode for Tegra30. It supports the
> secondary CPUs (i.e., CPU1-CPU3) to go into LP2 dynamically. When any of
> the secondary CPUs go into LP2, it can be power gated alone. There is a
> limitation on CPU0. The CPU0 can go into LP2 only when all secondary CPUs
> are already in LP2. After CPU0 is in LP2, the CPU rail can be turned off.
>
> Verified on Seaboard(Tegra20) and Cardhu(Tegra30).
What's the most comprehensive way to verify this? I booted Cardhu with
these patches applied and saw that all CPU cores did enter both idle
states. However, I'm unsure what the best way to stress the system is,
i.e. how would I stress and test for correct handling of all the L2
caching/SMP coherency issues, etc.
^ permalink raw reply
* [PATCH 2/7] ARM: tegra: cpuidle: add LP2 resume function
From: Stephen Warren @ 2012-10-09 22:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1349691981-31038-3-git-send-email-josephl@nvidia.com>
On 10/08/2012 04:26 AM, Joseph Lo wrote:
> LP2 is one of the Tegra low power states that supports power gating both
> CPU cores and GICs. Adding a resume function for taking care the CPUs that
> resume from LP2. This function was been hooked to reset handler. We take
> care everything here before go into kernel.
> diff --git a/arch/arm/mach-tegra/headsmp.S b/arch/arm/mach-tegra/headsmp.S
> +ENTRY(tegra_resume)
...
> + /* Are we on Tegra20? */
> + mov32 r6, TEGRA_APB_MISC_BASE
> + ldr r0, [r6, #APB_MISC_GP_HIDREV]
> + and r0, r0, #0xff00
> + cmp r0, #(0x20 << 8)
> + beq 1f
> +#ifdef CONFIG_ARCH_TEGRA_3x_SOC
> + /* Clear the flow controller flags for this CPU. */
> + mov32 r2, TEGRA_FLOW_CTRL_BASE + FLOW_CTRL_CPU0_CSR @ CPU0 CSR
> + ldr r1, [r2]
> + /* Clear event & intr flag */
> + orr r1, r1, \
> + #FLOW_CTRL_CSR_INTR_FLAG | FLOW_CTRL_CSR_EVENT_FLAG
> + movw r0, #0x0FFD @ enable, cluster_switch, immed, & bitmaps
> + bic r1, r1, r0
> + str r1, [r2]
> +#endif
> +1:
couldn't that entire quoted chunk go inside the ifdef; all of reading
the HIDREV register and the 1: label?
^ permalink raw reply
* [PATCH 3/7] ARM: tegra30: cpuidle: add LP2 driver for secondary CPUs
From: Stephen Warren @ 2012-10-09 22:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1349691981-31038-4-git-send-email-josephl@nvidia.com>
On 10/08/2012 04:26 AM, Joseph Lo wrote:
> This supports power-gated (LP2) idle on secondary CPUs for Tegra30.
> The secondary CPUs can go into LP2 state independently. When CPU goes
> into LP2 state, it saves it's state and puts itself to flow controlled
> WFI state. After that, it will been power gated.
> diff --git a/arch/arm/mach-tegra/cpuidle-tegra30.c b/arch/arm/mach-tegra/cpuidle-tegra30.c
> static struct cpuidle_driver tegra_idle_driver = {
> .name = "tegra_idle",
> .owner = THIS_MODULE,
> .en_core_tk_irqen = 1,
> - .state_count = 1,
> + .state_count = 2,
Doesn't that assignment need to be ifdef'd just like the array entry
setup below:
> .states = {
> [0] = ARM_CPUIDLE_WFI_STATE_PWR(600),
> +#ifdef CONFIG_PM_SLEEP
> + [1] = {
> + .enter = tegra30_idle_lp2,
> + .exit_latency = 2000,
> + .target_residency = 2200,
> + .power_usage = 0,
> + .flags = CPUIDLE_FLAG_TIME_VALID,
> + .name = "LP2",
> + .desc = "CPU power-gate",
> + },
> +#endif
> },
> };
> @@ -41,6 +114,10 @@ int __init tegra30_cpuidle_init(void)
> struct cpuidle_device *dev;
> struct cpuidle_driver *drv = &tegra_idle_driver;
>
> +#ifndef CONFIG_PM_SLEEP
> + drv->state_count = 1; /* support clockgating only */
> +#endif
Oh, I see it's done here. Just fixing the static initialization seems a
lot simpler?
> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
> +void __cpuinit tegra_clear_cpu_in_lp2(int cpu)
> +{
> + spin_lock(&tegra_lp2_lock);
> + BUG_ON(!cpumask_test_cpu(cpu, &tegra_in_lp2));
> + cpumask_clear_cpu(cpu, &tegra_in_lp2);
> +
> + /*
> + * Update the IRAM copy used by the reset handler. The IRAM copy
> + * can't use used directly by cpumask_clear_cpu() because it uses
> + * LDREX/STREX which requires the addressed location to be inner
> + * cacheable and sharable which IRAM isn't.
> + */
> + writel(tegra_in_lp2.bits[0], tegra_cpu_lp2_mask);
> + dsb();
Why not /just/ store the data in IRAM, and read/write directly to it,
rather than maintaining an SDRAM-based copy of it?
Then, wouldn't the body of this function be simply:
spin_lock();
BUG_ON(!(tegra_cpu_lp2_mask & BIT(cpu)));
tegra_cpu_lp2_mask |= BIT(cpu);
spin_unlock();
> +bool __cpuinit tegra_set_cpu_in_lp2(int cpu)
Similar comment here.
^ permalink raw reply
* [PATCH 4/7] ARM: tegra30: common: enable csite clock
From: Stephen Warren @ 2012-10-09 22:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1349691981-31038-5-git-send-email-josephl@nvidia.com>
On 10/08/2012 04:26 AM, Joseph Lo wrote:
> Enable csite (debug and trace controller) clock at init to prevent it
> be disabled. And this also the necessary clock for CPU be brought up or
> resumed from a power-gate low power state (e.g., LP2).
Does it make sense to enable this clock only when entering LP2? Or do we
really need to keep it on 100% of the time?
^ permalink raw reply
* [PATCH 7/7] ARM: tegra30: cpuidle: add LP2 driver for CPU0
From: Stephen Warren @ 2012-10-09 22:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1349691981-31038-8-git-send-email-josephl@nvidia.com>
On 10/08/2012 04:26 AM, Joseph Lo wrote:
> The cpuidle LP2 is a power gating idle mode. It support power gating
> vdd_cpu rail after all cpu cores in LP2. For Tegra30, the CPU0 must
> be last one to go into LP2. We need to take care and make sure whole
> secondary CPUs were in LP2 by checking CPU and power gate status.
> After that, the CPU0 can go into LP2 safely. Then power gating the
> CPU rail.
> diff --git a/arch/arm/mach-tegra/cpuidle-tegra30.c b/arch/arm/mach-tegra/cpuidle-tegra30.c
> +static bool tegra30_idle_enter_lp2_cpu_0(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv,
> + int index)
> +{
> + struct cpuidle_state *state = &drv->states[index];
> + u32 cpu_on_time = state->exit_latency;
> + u32 cpu_off_time = state->target_residency - state->exit_latency;
> +
> + if (num_online_cpus() > 1 && !tegra_cpu_rail_off_ready()) {
Should that be || not &&?
Isn't the "num_online_cpus() > 1" condition effectively checked at the
call site, i.e. in tegra30_idle_lp2() below via the if (last_cpu) check?
> @@ -85,16 +108,22 @@ static int __cpuinit tegra30_idle_lp2(struct cpuidle_device *dev,
> int index)
> {
> bool entered_lp2 = false;
> + bool last_cpu;
>
> local_fiq_disable();
>
> + last_cpu = tegra_set_cpu_in_lp2(dev->cpu);
> + if (dev->cpu == 0) {
> + if (last_cpu)
> + entered_lp2 = tegra30_idle_enter_lp2_cpu_0(dev, drv,
> + index);
> + else
> + cpu_do_idle();
> + } else {
> entered_lp2 = tegra30_idle_enter_lp2_cpu_n(dev, drv, index);
> + }
Hmm. That means that if the last CPU to enter LP2 is e.g. CPU1, then
even though all CPUs are now in LP2, the complex as a whole doesn't
enter LP2. Is there a way to make the cluster as a whole enter LP2 in
this case? Isn't that what coupled cpuidle is for?
> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
> +static void set_power_timers(unsigned long us_on, unsigned long us_off)
> + if (tegra_pclk == NULL) {
> + tegra_pclk = clk_get_sys(NULL, "pclk");
> + if (IS_ERR(tegra_pclk)) {
> + /*
> + * pclk not been init or not exist.
> + * Use sclk to take the place of it.
> + * The default setting was pclk=sclk.
> + */
> + tegra_pclk = clk_get_sys(NULL, "sclk");
> + }
> + }
That's a little odd. Surely the HW has pclk or it doesn't? Why use
different clocks at different times for what is apparently the same thing?
^ permalink raw reply
* [PATCH 1/6] pm: add suspend_mem and suspend_standby support
From: Rafael J. Wysocki @ 2012-10-09 22:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1349594840-11374-1-git-send-email-plagnioj@jcrosoft.com>
On Sunday 07 of October 2012 09:27:15 Jean-Christophe PLAGNIOL-VILLARD wrote:
> Today when we go to suspend we can not knwon at drivers level if we go in
> STANDBY or MEM. Fix this by introducing two new callback suspend_mem and
> suspend_standby.
>
> If a bus does not need to care about this distinction we fallback to
> suspend. This will allow to do not update the current bus or drivers.
>
> This is needed as example by at91 OHCI and atmel serial
>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: linux-pm at vger.kernel.org
So for me, this patch is a total no-go.
Apart from the fact that I don't see any difference between .suspend()
and .suspend_mem(), it doesn't cover the late/early and noirq phases.
> ---
> drivers/base/power/main.c | 12 ++++++++++++
> include/linux/pm.h | 9 +++++++++
> kernel/power/suspend.c | 16 ++++++++++++++--
> 3 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index a3c1404..581e135 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -218,9 +218,21 @@ static void dpm_wait_for_children(struct device *dev, bool async)
> */
> static pm_callback_t pm_op(const struct dev_pm_ops *ops, pm_message_t state)
> {
> + pm_callback_t callback = NULL;
> +
> switch (state.event) {
> #ifdef CONFIG_SUSPEND
> case PM_EVENT_SUSPEND:
> + switch (state.param) {
> + case PM_SUSPEND_MEM:
> + callback = ops->suspend_mem;
> + break;
> + case PM_SUSPEND_STANDBY:
> + callback = ops->suspend_standby;
> + break;
> + }
> + if (callback)
> + return callback;
> return ops->suspend;
> case PM_EVENT_RESUME:
> return ops->resume;
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 007e687..aff344b 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -49,6 +49,7 @@ extern const char power_group_name[]; /* = "power" */
>
> typedef struct pm_message {
> int event;
> + int param;
> } pm_message_t;
Please don't do that. We have the struct platform_suspend_ops with callbacks
that take the state argument and we pass the information about the system sleep
state being entered in there. The way this information is interpreted depends
on the platform and the actions of device drivers resulting from that must be
platform-dependent too. Your design makes a very strong assumption that the
interpretation of "standby" by the platform and device drivers will always
be the same, which generally need not be the case.
[And what if the given platform actually has more than two sleep states?
Are you going to add a separate callback for each of them?]
Make your drivers ask the platform what power state to put the devices into
instead of doing this.
> /**
> @@ -265,6 +266,8 @@ struct dev_pm_ops {
> int (*prepare)(struct device *dev);
> void (*complete)(struct device *dev);
> int (*suspend)(struct device *dev);
> + int (*suspend_mem)(struct device *dev);
> + int (*suspend_standby)(struct device *dev);
> int (*resume)(struct device *dev);
> int (*freeze)(struct device *dev);
> int (*thaw)(struct device *dev);
> @@ -295,8 +298,12 @@ struct dev_pm_ops {
> .thaw = resume_fn, \
> .poweroff = suspend_fn, \
> .restore = resume_fn,
> +#define SET_SYSTEM_SLEEP_STANDBY_MEM_PM_OPS(suspend_standby_fn, suspend_mem_fn) \
> + .suspend_standby = suspend_standby_fn, \
> + .suspend_mem = suspend_mem_fn,
> #else
> #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
> +#define SET_SYSTEM_SLEEP_STANDBY_MEM_PM_OPS(suspend_standby_fn, suspend_mem_fn)
> #endif
>
> #ifdef CONFIG_PM_RUNTIME
> @@ -414,6 +421,8 @@ const struct dev_pm_ops name = { \
> #define PMSG_FREEZE ((struct pm_message){ .event = PM_EVENT_FREEZE, })
> #define PMSG_QUIESCE ((struct pm_message){ .event = PM_EVENT_QUIESCE, })
> #define PMSG_SUSPEND ((struct pm_message){ .event = PM_EVENT_SUSPEND, })
> +#define PMSG_SUSPEND_MEM ((struct pm_message){ .event = PM_EVENT_SUSPEND, .param = PM_SUSPEND_MEM, })
> +#define PMSG_SUSPEND_STANDBY ((struct pm_message){ .event = PM_EVENT_SUSPEND, .param = PM_SUSPEND_STANDBY, })
> #define PMSG_HIBERNATE ((struct pm_message){ .event = PM_EVENT_HIBERNATE, })
> #define PMSG_RESUME ((struct pm_message){ .event = PM_EVENT_RESUME, })
> #define PMSG_THAW ((struct pm_message){ .event = PM_EVENT_THAW, })
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index c8b7446..9505b55 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -126,6 +126,18 @@ void __attribute__ ((weak)) arch_suspend_enable_irqs(void)
> local_irq_enable();
> }
>
> +static pm_message_t suspend_state_to_pm_state(suspend_state_t state)
> +{
> + switch (state) {
> + case PM_SUSPEND_STANDBY:
> + return PMSG_SUSPEND_STANDBY;
> + case PM_SUSPEND_MEM:
> + return PMSG_SUSPEND_MEM;
> + default:
> + return PMSG_SUSPEND;
> + }
> +}
Well, this is more than ugly.
> +
> /**
> * suspend_enter - Make the system enter the given sleep state.
> * @state: System sleep state to enter.
> @@ -143,7 +155,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
> goto Platform_finish;
> }
>
> - error = dpm_suspend_end(PMSG_SUSPEND);
> + error = dpm_suspend_end(suspend_state_to_pm_state(state));
> if (error) {
> printk(KERN_ERR "PM: Some devices failed to power down\n");
> goto Platform_finish;
> @@ -215,7 +227,7 @@ int suspend_devices_and_enter(suspend_state_t state)
> suspend_console();
> ftrace_stop();
> suspend_test_start();
> - error = dpm_suspend_start(PMSG_SUSPEND);
> + error = dpm_suspend_start(suspend_state_to_pm_state(state));
> if (error) {
> printk(KERN_ERR "PM: Some devices failed to suspend\n");
> goto Recover_platform;
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* [PATCH 1/6] ARM: bcm476x: Add infrastructure
From: Domenico Andreoli @ 2012-10-09 22:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201210090918.56725.arnd@arndb.de>
On Tue, Oct 09, 2012 at 09:18:56AM +0000, Arnd Bergmann wrote:
> Hi Domenico,
Hello,
> Your series looks very nice, thanks for the contribution!
hehe.. thanks also to the hard work committed in the ARM sub-tree in these
past (few!) months.
> Stephen has already covered everything I would have commented and
> more, except for one thing that I found:
>
> On Sunday 07 October 2012, Domenico Andreoli wrote:
> > Index: b/arch/arm/boot/dts/bcm476x.dtsi
> > ===================================================================
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/bcm476x.dtsi
> > @@ -0,0 +1,31 @@
> > +/include/ "skeleton.dtsi"
> > +
> > +/ {
> > + compatible = "brcm,bcm476x";
> > + model = "Broadcom BCM476x";
> > +
> > ...
> > + vic0: interrupt-controller at 80000 {
> > + compatible = "brcm,bcm476x-pl192", "arm,pl192-vic", "arm,primecell";
> > + reg = <0x80000 0x1000>;
> > + interrupt-controller;
> > + #interrupt-cells = <1>;
> > + };
>
> I suppose that the name of the soc is not actually "bcm476x" but you
> are in fact referring to the family including bcm4760 and bcm4761.
Yes, those two.
> The convention in the device tree is to always use specific product
> numbers, rather than wildcards. If one of the two has a superset of
> the hardware of the other, I would recommend you pick that number
> in the device tree and in the drivers, and for the other one, you
> just mark them as compatible to both of them.
bcm4760 is the superset (bcm4761 hasn't the gpu) but there are also other
differences (that I know, USB phy and clocks).
I can identify the SoC at runtime, does this change anything to your
recommendation?
Thanks for the feedback.
Regards,
Domenico
^ permalink raw reply
* [PATCH 2/6] ARM: bcm476x: Add system timer
From: Domenico Andreoli @ 2012-10-09 23:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50738F48.5010002@wwwdotorg.org>
On Mon, Oct 08, 2012 at 08:43:20PM -0600, Stephen Warren wrote:
> On 10/08/2012 05:50 AM, Florian Fainelli wrote:
> > On Sunday 07 October 2012 03:53:02 Domenico Andreoli wrote:
> >> From: Domenico Andreoli <domenico.andreoli@linux.com>
> >>
> >> System timer implementation for the BCM476x SoCs.
>
> >> Index: b/arch/arm/boot/dts/bcm476x.dtsi
>
> >> + timer {
> >> + compatible = "brcm,bcm476x-system-timer";
> >> + reg = <0xba000 0x1000>;
> >> + interrupt-parent = <&vic0>;
> >> + interrupts = <4>, <11>;
> >> + clock-frequency = <24000000>;
> >> + };
> >
> > This should rather be timer: timer at ba0000 for this node to be uniquely named.
>
> I only see 1 timer node in this series, so "timer" is already unique,
> isn't it?
I don't mind to add the address, this SoC actually has three timer blocks.
Dome
^ permalink raw reply
* [GIT PULL 1/5] omap fixes for v3.7-rc1
From: Olof Johansson @ 2012-10-09 23:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50747195.25cf320a.259b.ffffd04bSMTPIN_ADDED@mx.google.com>
Hi,
On Tue, Oct 9, 2012 at 11:46 AM, Tony Lindgren <tony@atomide.com> wrote:
> The following changes since commit 5e090ed7af10729a396a25df43d69a236e789736:
>
> Merge tag 'soc-late' of git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc (2012-10-07 20:55:16 +0900)
>
> are available in the git repository at:
>
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap tags/omap-for-v3.7-rc1/fixes-signed
Applied 1-5. Thanks!
-Olof
^ permalink raw reply
* IRQ code inconsistencies across platforms
From: linux at prisktech.co.nz @ 2012-10-09 23:26 UTC (permalink / raw)
To: linux-arm-kernel
In arch/arm/common.gic.c:
static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
urq_hw_number_t hw)
{
...
irq_set_chip_and_handler(irq, &gic_chip, handle_fasteoi_irq);
set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
}
but in arch/arm/mach-pxa/irq.c:
static int pxa_irq_map(struct irq_domain *d, unsigned int virq, urq_hw_number_t hw)
{
...
irq_set_chip_and_handler(hw, &pxa_internal_irq_chip, handle_level_irq);
set_irq_flags(hw, IRQF_VALID);
}
Is it correct to use irq/virq or hw for irq_set_chip_and_handler/set_irq_flags?
Both platforms are adding legacy domains, so while they both presumably work,
one is slightly incorrect.
Regards
Tony P
^ permalink raw reply
* [PATCH 6/6] ARM: bcm476x: Instantiate console UART
From: Domenico Andreoli @ 2012-10-09 23:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <507394CB.5080300@wwwdotorg.org>
On Mon, Oct 08, 2012 at 09:06:51PM -0600, Stephen Warren wrote:
> On 10/06/2012 07:53 PM, Domenico Andreoli wrote:
> > From: Domenico Andreoli <domenico.andreoli@linux.com>
> >
> > The BCM476x sports standard PL011 UARTs which are fully described and
> > put to use with these few changes.
>
> Given that this SoC uses an interrupt controller for which there's
> already a driver (so there's no need to wait for a later patch before
> interrupts work), and that patch 1/6 adds debug/bcm476x.S anyway, I'd be
> inclined to just squash this patch into the very first one myself.
>
> If not, this patch isn't really just instantiating the console UART, but
> also allowing the DEBUG_LL implementation to be selected.
I created a separate patch for the DEBUG_LL and the rest is squashed in
the first infrastructure patch.
Regards,
Domenico
^ permalink raw reply
* [PATCH 0/6] ARM: Add support for Broadcom BCM476x SoCs
From: Domenico Andreoli @ 2012-10-09 23:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50738F8F.3030006@wwwdotorg.org>
On Mon, Oct 08, 2012 at 08:44:31PM -0600, Stephen Warren wrote:
> On 10/07/2012 04:14 AM, Domenico Andreoli wrote:
> > On Sat, Oct 06, 2012 at 11:22:18PM -0600, Stephen Warren wrote:
> >> On 10/06/2012 07:53 PM, Domenico Andreoli wrote:
> >>>
> >>> Additional support is being worked on.... and usb (DWC OTG) only
> >>> attemped reusing s3c-hsotg.
> >>
> >> Hmmm. I believe the bcm2835 uses DWC OTG. I wonder if the same driver
> >> will work there; USB in particular is a major pain point on the bcm2835...
> >
> > I don't know any more how many versions of this driver are circulating,
> > we may have some options.
> >
> >>> It's based on a random pre v3.7-rc1 commit (eb0ad9c) with mainlined
> >>> multi-platform support and Stephen's patch to add DEBUG_LL to it.
> >>
> >> ...
> >> about uncompress.h now. Unless anyone really pipes up and says they'd
> >> still like me to pursue uncompress.h in multi-platform, I'm inclined to
> >> drop it.
> >
> > I think it is valuable, multi-platform doesn't have any DEBUG_LL or
> > earlyprintk support
>
> Why do you say that? Locally I have ported Tegra to multi-platform and I
> certainly have earlyprintk enabled, and I dropped the patches to support
> uncompress.h in multi-platform from my local work branch.
Yes, sorry, it's me. I tend to mix DEBUG_LL/earlyprintk with the decompressor
output. Only the latter is broken on multi-platform.
cheers,
Domenico
^ permalink raw reply
* [PATCH 1/2] ARM: OMAP: Trivial driver changes to remove include plat/cpu.h
From: Tony Lindgren @ 2012-10-10 0:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5073E84E.4090408@ti.com>
* P?ter Ujfalusi <peter.ujfalusi@ti.com> [121009 02:03]:
> On 10/08/2012 07:35 PM, Tony Lindgren wrote:
>
> > - omap-dma.c and omap-pcm.c can test the arch locally as
> > omap1 and omap2 cannot be compiled together because of
> > conflicting compiler flags
>
> > sound/soc/omap/omap-pcm.c | 9 +++++++--
>
> Tony: is this going to be included in 3.7?
Hmm I guess we could try to get this out of the way
to cut down the dependencies. Let's if maintainers
of the other affected drivers this is OK for the
-rc series.
Regards,
Tony
^ permalink raw reply
* [kvmarm] [PATCH v2 06/14] KVM: ARM: Memory virtualization setup
From: Min-gyu Kim @ 2012-10-10 1:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <6994599196536c9557528c465f8f93b0@localhost>
> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier at arm.com]
> Sent: Tuesday, October 09, 2012 9:56 PM
> To: Christoffer Dall
> Cc: Min-gyu Kim; ???; linux-arm-kernel at lists.infradead.org;
> kvm at vger.kernel.org; kvmarm at lists.cs.columbia.edu
> Subject: Re: [kvmarm] [PATCH v2 06/14] KVM: ARM: Memory virtualization
> setup
>
> On Sat, 6 Oct 2012 17:33:43 -0400, Christoffer Dall
> <c.dall@virtualopensystems.com> wrote:
> > On Thu, Oct 4, 2012 at 10:23 PM, Min-gyu Kim
> > <mingyu84.kim@samsung.com>
> > wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: kvm-owner at vger.kernel.org [mailto:kvm-owner at vger.kernel.org]
> >>> On Behalf Of Christoffer Dall
> >>> Sent: Monday, October 01, 2012 6:11 PM
> >>> To: kvm at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> >>> kvmarm at lists.cs.columbia.edu
> >>> Cc: Marc Zyngier
> >>> Subject: [PATCH v2 06/14] KVM: ARM: Memory virtualization setup
> >>>
> >>> +static void stage2_set_pte(struct kvm *kvm, struct
> kvm_mmu_memory_cache
> >>> *cache,
> >>> + phys_addr_t addr, const pte_t *new_pte) {
> >>> + pgd_t *pgd;
> >>> + pud_t *pud;
> >>> + pmd_t *pmd;
> >>> + pte_t *pte, old_pte;
> >>> +
> >>> + /* Create 2nd stage page table mapping - Level 1 */
> >>> + pgd = kvm->arch.pgd + pgd_index(addr);
> >>> + pud = pud_offset(pgd, addr);
> >>> + if (pud_none(*pud)) {
> >>> + if (!cache)
> >>> + return; /* ignore calls from kvm_set_spte_hva */
> >>> + pmd = mmu_memory_cache_alloc(cache);
> >>> + pud_populate(NULL, pud, pmd);
> >>> + pmd += pmd_index(addr);
> >>> + get_page(virt_to_page(pud));
> >>> + } else
> >>> + pmd = pmd_offset(pud, addr);
> >>> +
> >>> + /* Create 2nd stage page table mapping - Level 2 */
> >>> + if (pmd_none(*pmd)) {
> >>> + if (!cache)
> >>> + return; /* ignore calls from kvm_set_spte_hva */
> >>> + pte = mmu_memory_cache_alloc(cache);
> >>> + clean_pte_table(pte);
> >>> + pmd_populate_kernel(NULL, pmd, pte);
> >>> + pte += pte_index(addr);
> >>> + get_page(virt_to_page(pmd));
> >>> + } else
> >>> + pte = pte_offset_kernel(pmd, addr);
> >>> +
> >>> + /* Create 2nd stage page table mapping - Level 3 */
> >>> + old_pte = *pte;
> >>> + set_pte_ext(pte, *new_pte, 0);
> >>> + if (pte_present(old_pte))
> >>> + __kvm_tlb_flush_vmid(kvm);
> >>> + else
> >>> + get_page(virt_to_page(pte)); }
> >>
> >>
> >> I'm not sure about the 3-level page table, but isn't it necessary to
> >> clean the page table for 2nd level?
> >> There are two mmu_memory_cache_alloc calls. One has following
> >> clean_pte_table and the other doesn't have.
> >
> > hmm, it probably is - I couldn't really find the common case where
> > this is done in the kernel normally (except for some custom loop in
> > ioremap and idmap), but I added this fix:
> >
> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c index
> > 5394a52..f11ba27f 100644
> > --- a/arch/arm/kvm/mmu.c
> > +++ b/arch/arm/kvm/mmu.c
> > @@ -430,6 +430,7 @@ static void stage2_set_pte(struct kvm *kvm, struct
> > kvm_mmu_memory_cache *cache,
> > if (!cache)
> > return; /* ignore calls from kvm_set_spte_hva */
> > pmd = mmu_memory_cache_alloc(cache);
> > + clean_dcache_area(pmd, PTRS_PER_PMD * sizeof(pmd_t));
> > pud_populate(NULL, pud, pmd);
> > pmd += pmd_index(addr);
> > get_page(virt_to_page(pud));
>
> There ought to be a test of ID_MMFR3[23:20] to find out whether or not it
> is useful to clean the dcache. Not sure if that's a massive gain, but it
> is certainly an optimisation to consider for the kernel as a whole.
That's part of what I was wondering. clean_dcache_area is substituted by nop if
TLB_CAN_READ_FROM_L1_CACHE is defined(mm/proc-v7.S).
But I couldn't find any place where it is defined.
If it is part of bsp to enable TLB_CAN_RAD_FROM_L1_CACHE according to ID_MMFR3[23:20],
it would not be necessary to concerned about cleaning or not cleaning.
However, am I right?
>
> M.
> --
> Fast, cheap, reliable. Pick two.
^ permalink raw reply
* [PATCH v2 7/8] video: mark nuc900fb_map_video_memory as __devinit
From: Wan ZongShun @ 2012-10-10 1:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1349813638-4617-8-git-send-email-arnd@arndb.de>
2012/10/10 Arnd Bergmann <arnd@arndb.de>:
> nuc900fb_map_video_memory is called by an devinit function
> that may be called at run-time, but the function itself is
> marked __init and will be discarded after boot.
>
> To avoid calling into a function that may have been overwritten,
> mark nuc900fb_map_video_memory itself as __devinit.
>
> Without this patch, building nuc950_defconfig results in:
>
> WARNING: drivers/video/built-in.o(.devinit.text+0x26c): Section mismatch in reference from the function nuc900fb_probe() to the function .init.text:nuc900fb_map_video_memory()
> The function __devinit nuc900fb_probe() references
> a function __init nuc900fb_map_video_memory().
> If nuc900fb_map_video_memory is only used by nuc900fb_probe then
> annotate nuc900fb_map_video_memory with a matching annotation.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Wan ZongShun <mcuos.com@gmail.com>
> Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> Cc: linux-fbdev at vger.kernel.org
Thanks for your patch.
Acked-by: Wan ZongShun <mcuos.com@gmail.com>
> ---
> drivers/video/nuc900fb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/nuc900fb.c b/drivers/video/nuc900fb.c
> index e10f551..b31b12b 100644
> --- a/drivers/video/nuc900fb.c
> +++ b/drivers/video/nuc900fb.c
> @@ -387,7 +387,7 @@ static int nuc900fb_init_registers(struct fb_info *info)
> * The buffer should be a non-cached, non-buffered, memory region
> * to allow palette and pixel writes without flushing the cache.
> */
> -static int __init nuc900fb_map_video_memory(struct fb_info *info)
> +static int __devinit nuc900fb_map_video_memory(struct fb_info *info)
> {
> struct nuc900fb_info *fbi = info->par;
> dma_addr_t map_dma;
> --
> 1.7.10
>
--
Wan ZongShun.
www.mcuos.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox