Re: Clocks used by another OS/CPU (was: Re: [RFC PATCH] clk: renesas: cpg-mssr: Add interface for critical core clocks)
On Mon, Jul 03, 2017 at 10:17:22AM +0100, Sudeep Holla wrote: > > > On 01/07/17 19:14, Uwe Kleine-König wrote: > > Hello, > > > > On Sat, Jul 01, 2017 at 07:02:48AM +0200, Dirk Behme wrote: > > [...] > > >> > >> > >> The other problem is security related. If, at all, you have to do it the > >> other way around, then: > >> > >> Make Linux a consumer of the other CPU's (trusted/trustzone/whatever > >> secured) OS clock driver. > > Yes, that's better and is getting common on newer platforms. They have > separate M-class(or even low A-class e.g. A5/A7) processors to handle > all the system management. > > The new ARM SCMI specification[0][1] is designed to standardize the > interface. It covers the clocks in clock protocol. > Yes, however this doesn't exist on older SoCs which still have multiple CPU's Peter.
Re: Clocks used by another OS/CPU (was: Re: [RFC PATCH] clk: renesas: cpg-mssr: Add interface for critical core clocks)
On 04/07/17 08:31, Peter De Schrijver wrote: > On Mon, Jul 03, 2017 at 10:17:22AM +0100, Sudeep Holla wrote: >> >> >> On 01/07/17 19:14, Uwe Kleine-König wrote: >>> Hello, >>> >>> On Sat, Jul 01, 2017 at 07:02:48AM +0200, Dirk Behme wrote: >> >> [...] >> The other problem is security related. If, at all, you have to do it the other way around, then: Make Linux a consumer of the other CPU's (trusted/trustzone/whatever secured) OS clock driver. >> >> Yes, that's better and is getting common on newer platforms. They have >> separate M-class(or even low A-class e.g. A5/A7) processors to handle >> all the system management. >> >> The new ARM SCMI specification[0][1] is designed to standardize the >> interface. It covers the clocks in clock protocol. >> > > Yes, however this doesn't exist on older SoCs which still have multiple CPU's > Agreed. But if someone is fixing/adding support in Linux as well as in the other OS running on those cores, why not consider this interface instead of trying to generalize something which will invariably SoC specific. -- Regards, Sudeep
Re: [RFT] mmc: tmio: fix CMD12 (STOP) handling
On Mon, Jul 03, 2017 at 09:28:23PM +0200, Wolfram Sang wrote:
> I always anticipated this code to be not correct, but now I had a test
> case to prove it. According to all documentation I have, setting the
> TMIO_STOP_STP bit ever only worked during block transfers. This bit is
> like manually enforcing an autocmd12 during a so far seamless transfer.
> It does NOT work when the block transfer had errors. It also does NOT
> work with any other cmd except block commands. For all those, CMD12 has
> to be treated like any other command. So, basically, we could use this
> bit only for mrq->data->stop cmds. But for these, we happily use the
> autocmd12 feature using the TMIO_STOP_SEC bit. As a result, the above
> bit is not useful for us and we need to treat CMD12 as a regular cmd
> always. Just remove the special handling code. Note that the BSP
> recognized this issue as well yet had a more cautious solution to the
> problem [1]. Which is understandable but makes CMD12 handling even more
> complicated.
>
> Checked with a Renesas Salvator-X/M3-W which needed to send CMD12 when
> retuning one of my SD cards.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=2838a2ff8ca776f6d18b7fbbe75f3df8dd64183a
>
> Signed-off-by: Wolfram Sang
> ---
>
> So, this is a friendly call for further testing to make sure no regressions
> happen. I also put the authors of the BSP patch to CC to check if this patch
> also matches their use case. I hope this is fine for these people, please
> accept my apologies if not. I just really like to have your opinion on this
> patch.
We've stumbled upon this issue some months ago and the patch below was
exactly the one that I came up with too. We've provided the feedback to
Renesas and they fixed it as [1]. As I'm laking proper data sheets for
the controller I was not able to judge it in depth, though.
Unfortunately I was not able to reproduce the original issue to test the
Renesas version [1] with enough confidence. What I can tell with
confidence is that the patch below does fix the issue that the driver
gets stuck on CMD12 and it did not have any further negative side
effects. So from my side (tested on RCar-M3):
Tested-by: Jan Klötzke
Maybe someone from Reseas can comment on the rationale of their version
of the patch.
>
> Geert, Simon: any chance you can run this patch on your board farms?
>
> In any case: as far as my understanding goes, if I am wrong with my
> assumptions, the worst case is that for older SoCs CMD12 handling might become
> a tad more inefficient because we now do in software what was expected to be
> done in hardware. However, I am quite sure that the HW feature was always
> over-estimated and CMD12 support is simply broken. For my test case and the
> BSP
> patch above, it definately was.
>
> Thanks for any assistance,
>
>Wolfram
>
>
> drivers/mmc/host/tmio_mmc_core.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c
> b/drivers/mmc/host/tmio_mmc_core.c
> index 1851c883bfc82a..fbcd56c58bce24 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -355,12 +355,6 @@ static int tmio_mmc_start_command(struct tmio_mmc_host
> *host,
> int c = cmd->opcode;
> u32 irq_mask = TMIO_MASK_CMD;
>
> - /* CMD12 is handled by hardware */
> - if (cmd->opcode == MMC_STOP_TRANSMISSION && !cmd->arg) {
> - sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, TMIO_STOP_STP);
> - return 0;
> - }
> -
> switch (mmc_resp_type(cmd)) {
> case MMC_RSP_NONE: c |= RESP_NONE; break;
> case MMC_RSP_R1:
> --
> 2.11.0
>
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
Hi Krzysztof, Rafael,
On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski wrote:
> genpd_syscore_switch() had two problems:
> 1. It silently assumed that device, it is being called for, belongs to
>generic power domain and used container_of() on its power domain
>pointer. Such assumption might not be true always.
>
> 2. It iterated over list of generic power domains without holding
>gpd_list_lock mutex thus list could have been modified in the same
>time.
>
> Usage of genpd_lookup_dev() solves both problems as it is safe a call
> for non-generic power domains and uses mutex when iterating.
>
> Reported-by: Ulf Hansson
> Signed-off-by: Krzysztof Kozlowski
> Acked-by: Ulf Hansson
This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
request "[GIT PULL] Power management updates for v4.13-rc1".
> Not tested on real hardware.
This causes the following BUG during s2ram on all my Renesas arm32 boards,
where the system timer is an IRQ safe device:
PM: Syncing filesystems ... done.
PM: Preparing system for sleep (mem)
Freezing user space processes ... (elapsed 0.001 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
PM: Suspending system (mem)
PM: suspend of devices complete after 122.841 msecs
PM: suspend devices took 0.130 seconds
PM: late suspend of devices complete after 2.682 msecs
PM: noirq suspend of devices complete after 29.951 msecs
Disabling non-boot CPUs ...
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
in_atomic(): 0, irqs_disabled(): 128, pid: 1730, name: s2ram
CPU: 0 PID: 1730 Comm: s2ram Not tainted
4.12.0-koelsch-07061-g810fee9afeba15ef #3592
Hardware name: Generic R8A7791 (Flattened Device Tree)
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0x7c/0x9c)
[] (dump_stack) from [] (___might_sleep+0x124/0x160)
[] (___might_sleep) from [] (mutex_lock+0x18/0x60)
[] (mutex_lock) from [] (genpd_lookup_dev+0x38/0x94)
[] (genpd_lookup_dev) from []
(pm_genpd_syscore_poweroff+0x8/0x2c)
[] (pm_genpd_syscore_poweroff) from []
(sh_cmt_clock_event_suspend+0x18/0x28)
[] (sh_cmt_clock_event_suspend) from []
(clockevents_suspend+0x40/0x54)
[] (clockevents_suspend) from []
(timekeeping_suspend+0x23c/0x278)
[] (timekeeping_suspend) from []
(syscore_suspend+0x88/0x138)
[] (syscore_suspend) from []
(suspend_devices_and_enter+0x308/0x4fc)
[] (suspend_devices_and_enter) from []
(pm_suspend+0x228/0x280)
[] (pm_suspend) from [] (state_store+0xac/0xcc)
[] (state_store) from [] (kernfs_fop_write+0x170/0x1b0)
[] (kernfs_fop_write) from [] (__vfs_write+0x20/0x108)
[] (__vfs_write) from [] (vfs_write+0xb8/0x144)
[] (vfs_write) from [] (SyS_write+0x40/0x80)
[] (SyS_write) from [] (ret_fast_syscall+0x0/0x34)
Reverting the commit fixes the issue.
> ---
> drivers/base/power/domain.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 01e31d9f6c94..d31a4434b8b3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1098,8 +1098,8 @@ static void genpd_syscore_switch(struct device *dev,
> bool suspend)
> {
> struct generic_pm_domain *genpd;
>
> - genpd = dev_to_genpd(dev);
> - if (!pm_genpd_present(genpd))
> + genpd = genpd_lookup_dev(dev);
> + if (!genpd)
> return;
>
> if (suspend) {
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
renesas-drivers-2017-07-04-v4.12
I have pushed renesas-drivers-2017-07-04-v4.12 to
https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git
This tree is meant to ease development of platform support and drivers
for Renesas ARM SoCs. It is created by merging (a) the for-next branches
of various subsystem trees and (b) branches with driver code submitted
or planned for submission to maintainers into the development branch of
Simon Horman's renesas.git tree.
Today's version is based on renesas-devel-20170703-v4.12.
Included branches with driver code:
- topic/rza1-pfc-gpio-v6
- topic/suspend-to-idle-v1
- topic/sh-sh7722-sh7757-sh7264-sh7269-fix-pinctrl-v2
- topic/rcar2-cpg-mssr-dt-v2
- git://git.ragnatech.se/linux#for-renesas-drivers
-
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git#renesas/topic/i2c-core-dma
-
git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git#topic/sdhi-gen3-dma-2017-v3
- git://linuxtv.org/pinchartl/media.git#tags/drm-h3-es2-dt-20170626
- git://linuxtv.org/pinchartl/media.git#tags/drm-h3-es2-vsp-du-20170626
-
git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git#adv748x/driver
Included fixes:
- of_mdio: Fix broken PHY IRQ in case of probe deferral
- Revert "PM / Domains: Handle safely genpd_syscore_switch() call on
non-genpd device"
Included subsystem trees:
- git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git#linux-next
- git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git#clk-next
-
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git#for-next
- git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git#for-next
- git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git#for-next
- git://git.infradead.org/users/dedekind/l2-mtd-2.6.git#master
- git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git#master
- git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git#tty-next
- git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git#i2c/for-next
- git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git#for-next
- git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git#master
- git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git#usb-next
- git://people.freedesktop.org/~airlied/linux#drm-next
- git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git#next
- git://linuxtv.org/mchehab/media-next.git#master
- git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git#next
-
git://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git#for-next
- git://git.linaro.org/people/daniel.lezcano/linux.git#clockevents/next
- git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git#testing/next
- git://git.infradead.org/users/vkoul/slave-dma.git#next
-
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git#staging-next
- git://git.armlinux.org.uk/~rmk/linux-arm.git#for-next
- git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git#next
- git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git#for-next
- git://git.infradead.org/users/jcooper/linux.git#irqchip/for-next
- git://github.com/bzolnier/linux.git#fbdev-for-next
- git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata.git#for-next
-
git://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git#for-next
- git://www.linux-watchdog.org/linux-watchdog-next.git#master
- git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git#for-next
- git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git#for-next
- git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git#for-next/core
- git://anongit.freedesktop.org/drm/drm-misc#for-linux-next
- git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git#next
- git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git#next
-
git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git#next
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [RFT] mmc: tmio: fix CMD12 (STOP) handling
Hi Wolfram,
On Mon, Jul 3, 2017 at 9:28 PM, Wolfram Sang
wrote:
> I always anticipated this code to be not correct, but now I had a test
> case to prove it. According to all documentation I have, setting the
> TMIO_STOP_STP bit ever only worked during block transfers. This bit is
> like manually enforcing an autocmd12 during a so far seamless transfer.
> It does NOT work when the block transfer had errors. It also does NOT
> work with any other cmd except block commands. For all those, CMD12 has
> to be treated like any other command. So, basically, we could use this
> bit only for mrq->data->stop cmds. But for these, we happily use the
> autocmd12 feature using the TMIO_STOP_SEC bit. As a result, the above
> bit is not useful for us and we need to treat CMD12 as a regular cmd
> always. Just remove the special handling code. Note that the BSP
> recognized this issue as well yet had a more cautious solution to the
> problem [1]. Which is understandable but makes CMD12 handling even more
> complicated.
>
> Checked with a Renesas Salvator-X/M3-W which needed to send CMD12 when
> retuning one of my SD cards.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=2838a2ff8ca776f6d18b7fbbe75f3df8dd64183a
>
> Signed-off-by: Wolfram Sang
> ---
>
> So, this is a friendly call for further testing to make sure no regressions
> happen. I also put the authors of the BSP patch to CC to check if this patch
> also matches their use case. I hope this is fine for these people, please
> accept my apologies if not. I just really like to have your opinion on this
> patch.
>
> Geert, Simon: any chance you can run this patch on your board farms?
I've booted all my boards with your patch applied, no visible differences.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 0/7] ARM: dts: renesas: Add Inter Connect RAM
Hi Simon, Magnus,
R-Car Gen2 and RZ/G1 SoCs contain two or three blocks of SRAM, which can be
used for several purposes. One such purpose is holding a jump stub for CPU
core bringup.
This patch series adds the SRAM blocks to the various DTS files, following
the generic DT bindings for "mmio-sram" in
Documentation/devicetree/bindings/sram/sram.txt. Reserving SRAM for jump
stub for CPU core bringup will be handled in a follow-up series.
Thanks!
Geert Uytterhoeven (7):
ARM: dts: r8a7743: Add Inter Connect RAM
ARM: dts: r8a7745: Add Inter Connect RAM
ARM: dts: r8a7790: Add Inter Connect RAM
ARM: dts: r8a7791: Add Inter Connect RAM
ARM: dts: r8a7792: Add Inter Connect RAM
ARM: dts: r8a7793: Add Inter Connect RAM
ARM: dts: r8a7794: Add Inter Connect RAM
arch/arm/boot/dts/r8a7743.dtsi | 15 +++
arch/arm/boot/dts/r8a7745.dtsi | 15 +++
arch/arm/boot/dts/r8a7790.dtsi | 10 ++
arch/arm/boot/dts/r8a7791.dtsi | 10 ++
arch/arm/boot/dts/r8a7792.dtsi | 10 ++
arch/arm/boot/dts/r8a7793.dtsi | 10 ++
arch/arm/boot/dts/r8a7794.dtsi | 10 ++
7 files changed, 80 insertions(+)
--
2.7.4
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 7/7] ARM: dts: r8a7794: Add Inter Connect RAM
R-Car E2 has 2 regions of Inter Connect RAM (72 + 4 KiB).
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7794.dtsi | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi
index 7d9a81d970d87c6b..78973cee7185ffe6 100644
--- a/arch/arm/boot/dts/r8a7794.dtsi
+++ b/arch/arm/boot/dts/r8a7794.dtsi
@@ -588,6 +588,16 @@
status = "disabled";
};
+ icram0: sram@e63a {
+ compatible = "mmio-sram";
+ reg = <0 0xe63a 0 0x12000>;
+ };
+
+ icram1: sram@e63c {
+ compatible = "mmio-sram";
+ reg = <0 0xe63c 0 0x1000>;
+ };
+
ether: ethernet@ee70 {
compatible = "renesas,ether-r8a7794";
reg = <0 0xee70 0 0x400>;
--
2.7.4
[PATCH 1/7] ARM: dts: r8a7743: Add Inter Connect RAM
RZ/G1M has 3 regions of Inter Connect RAM (72 + 4 + 256 KiB).
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7743.dtsi | 15 +++
1 file changed, 15 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7743.dtsi b/arch/arm/boot/dts/r8a7743.dtsi
index 0ddac81742e4cdc7..b8ce935f6fb196fe 100644
--- a/arch/arm/boot/dts/r8a7743.dtsi
+++ b/arch/arm/boot/dts/r8a7743.dtsi
@@ -468,6 +468,21 @@
status = "disabled";
};
+ icram2: sram@e630 {
+ compatible = "mmio-sram";
+ reg = <0 0xe630 0 0x4>;
+ };
+
+ icram0: sram@e63a {
+ compatible = "mmio-sram";
+ reg = <0 0xe63a 0 0x12000>;
+ };
+
+ icram1: sram@e63c {
+ compatible = "mmio-sram";
+ reg = <0 0xe63c 0 0x1000>;
+ };
+
ether: ethernet@ee70 {
compatible = "renesas,ether-r8a7743";
reg = <0 0xee70 0 0x400>;
--
2.7.4
[PATCH 4/7] ARM: dts: r8a7791: Add Inter Connect RAM
R-Car M2-W has 2 regions of Inter Connect RAM (72 + 4 KiB).
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7791.dtsi | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index bd93f699ad840987..f4748a9cb0375e4d 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -890,6 +890,16 @@
status = "disabled";
};
+ icram0: sram@e63a {
+ compatible = "mmio-sram";
+ reg = <0 0xe63a 0 0x12000>;
+ };
+
+ icram1: sram@e63c {
+ compatible = "mmio-sram";
+ reg = <0 0xe63c 0 0x1000>;
+ };
+
ether: ethernet@ee70 {
compatible = "renesas,ether-r8a7791";
reg = <0 0xee70 0 0x400>;
--
2.7.4
[PATCH 6/7] ARM: dts: r8a7793: Add Inter Connect RAM
R-Car M2-N has 2 regions of Inter Connect RAM (72 + 4 KiB).
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7793.dtsi | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7793.dtsi b/arch/arm/boot/dts/r8a7793.dtsi
index 13b980f27bbc885f..bc6a44272f555215 100644
--- a/arch/arm/boot/dts/r8a7793.dtsi
+++ b/arch/arm/boot/dts/r8a7793.dtsi
@@ -848,6 +848,16 @@
status = "disabled";
};
+ icram0: sram@e63a {
+ compatible = "mmio-sram";
+ reg = <0 0xe63a 0 0x12000>;
+ };
+
+ icram1: sram@e63c {
+ compatible = "mmio-sram";
+ reg = <0 0xe63c 0 0x1000>;
+ };
+
ether: ethernet@ee70 {
compatible = "renesas,ether-r8a7793";
reg = <0 0xee70 0 0x400>;
--
2.7.4
[PATCH 5/7] ARM: dts: r8a7792: Add Inter Connect RAM
R-Car V2H has 2 regions of Inter Connect RAM (72 + 4 KiB).
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7792.dtsi | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7792.dtsi b/arch/arm/boot/dts/r8a7792.dtsi
index 0efecb232ee52ce0..136a86ac64974adb 100644
--- a/arch/arm/boot/dts/r8a7792.dtsi
+++ b/arch/arm/boot/dts/r8a7792.dtsi
@@ -465,6 +465,16 @@
status = "disabled";
};
+ icram0: sram@e63a {
+ compatible = "mmio-sram";
+ reg = <0 0xe63a 0 0x12000>;
+ };
+
+ icram1: sram@e63c {
+ compatible = "mmio-sram";
+ reg = <0 0xe63c 0 0x1000>;
+ };
+
sdhi0: sd@ee10 {
compatible = "renesas,sdhi-r8a7792";
reg = <0 0xee10 0 0x328>;
--
2.7.4
[PATCH 3/7] ARM: dts: r8a7790: Add Inter Connect RAM
R-Car H2 has 2 regions of Inter Connect RAM (72 + 4 KiB).
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7790.dtsi | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index 2805a8608d4ba007..4ee34995573cf505 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -830,6 +830,16 @@
status = "disabled";
};
+ icram0: sram@e63a {
+ compatible = "mmio-sram";
+ reg = <0 0xe63a 0 0x12000>;
+ };
+
+ icram1: sram@e63c {
+ compatible = "mmio-sram";
+ reg = <0 0xe63c 0 0x1000>;
+ };
+
ether: ethernet@ee70 {
compatible = "renesas,ether-r8a7790";
reg = <0 0xee70 0 0x400>;
--
2.7.4
[PATCH 2/7] ARM: dts: r8a7745: Add Inter Connect RAM
RZ/G1E has 3 regions of Inter Connect RAM (72 + 4 + 256 KiB).
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7745.dtsi | 15 +++
1 file changed, 15 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7745.dtsi b/arch/arm/boot/dts/r8a7745.dtsi
index 2feb0084bb3b1b51..88cf92bcd2f9b4fc 100644
--- a/arch/arm/boot/dts/r8a7745.dtsi
+++ b/arch/arm/boot/dts/r8a7745.dtsi
@@ -468,6 +468,21 @@
status = "disabled";
};
+ icram2: sram@e630 {
+ compatible = "mmio-sram";
+ reg = <0 0xe630 0 0x4>;
+ };
+
+ icram0: sram@e63a {
+ compatible = "mmio-sram";
+ reg = <0 0xe63a 0 0x12000>;
+ };
+
+ icram1: sram@e63c {
+ compatible = "mmio-sram";
+ reg = <0 0xe63c 0 0x1000>;
+ };
+
ether: ethernet@ee70 {
compatible = "renesas,ether-r8a7745";
reg = <0 0xee70 0 0x400>;
--
2.7.4
[PATCH 3/9] ARM: dts: r8a7743: Reserve SRAM for the SMP jump stub
Reserve SRAM for the jump stub for CPU core bringup.
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7743.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7743.dtsi b/arch/arm/boot/dts/r8a7743.dtsi
index b8ce935f6fb196fe..4d59e498fb45fe15 100644
--- a/arch/arm/boot/dts/r8a7743.dtsi
+++ b/arch/arm/boot/dts/r8a7743.dtsi
@@ -481,6 +481,14 @@
icram1: sram@e63c {
compatible = "mmio-sram";
reg = <0 0xe63c 0 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0 0xe63c 0x1000>;
+
+ smp-sram@0 {
+ compatible = "renesas,smp-sram";
+ reg = <0 0x10>;
+ };
};
ether: ethernet@ee70 {
--
2.7.4
[PATCH 2/9] ARM: shmobile: rcar-gen2: Obtain jump stub region from DT
Add support for obtaining from DT the SRAM region to store the jump stub
for CPU core bringup, according to the renesas,smp-sram DT bindings.
If no region is specified in DT, the code falls back to hardcoded ICRAM1
as before, to maintain backwards compatibility.
Signed-off-by: Geert Uytterhoeven
---
arch/arm/mach-shmobile/pm-rcar-gen2.c | 33 ++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-shmobile/pm-rcar-gen2.c
b/arch/arm/mach-shmobile/pm-rcar-gen2.c
index 0178da7ace82dcbd..e5f215c8b218100a 100644
--- a/arch/arm/mach-shmobile/pm-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/pm-rcar-gen2.c
@@ -11,7 +11,9 @@
*/
#include
+#include
#include
+#include
#include
#include
#include
@@ -69,8 +71,9 @@ void __init rcar_gen2_pm_init(void)
struct device_node *np, *cpus;
bool has_a7 = false;
bool has_a15 = false;
- phys_addr_t boot_vector_addr = ICRAM1;
+ struct resource res;
u32 syscier = 0;
+ int error;
if (once++)
return;
@@ -91,14 +94,38 @@ void __init rcar_gen2_pm_init(void)
else if (of_machine_is_compatible("renesas,r8a7791"))
syscier = 0x00111003;
+ np = of_find_compatible_node(NULL, NULL, "renesas,smp-sram");
+ if (!np) {
+ /* No smp-sram in DT, fall back to hardcoded address */
+ res = (struct resource)DEFINE_RES_MEM(ICRAM1,
+ shmobile_boot_size);
+ goto map;
+ }
+
+ error = of_address_to_resource(np, 0, &res);
+ if (error) {
+ pr_err("Failed to get smp-sram address: %d\n", error);
+ return;
+ }
+
+map:
/* RAM for jump stub, because BAR requires 256KB aligned address */
- p = ioremap_nocache(boot_vector_addr, shmobile_boot_size);
+ if (res.start & (256 * 1024 - 1) ||
+ resource_size(&res) < shmobile_boot_size) {
+ pr_err("Invalid smp-sram region\n");
+ return;
+ }
+
+ p = ioremap(res.start, resource_size(&res));
+ if (!p)
+ return;
+
memcpy_toio(p, shmobile_boot_vector, shmobile_boot_size);
iounmap(p);
/* setup reset vectors */
p = ioremap_nocache(RST, 0x63);
- bar = phys_to_sbar(boot_vector_addr);
+ bar = phys_to_sbar(res.start);
if (has_a15) {
writel_relaxed(bar, p + CA15BAR);
writel_relaxed(bar | SBAR_BAREN, p + CA15BAR);
--
2.7.4
[PATCH 9/9] ARM: dts: r8a7794: Reserve SRAM for the SMP jump stub
Reserve SRAM for the jump stub for CPU core bringup.
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7794.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi
index 78973cee7185ffe6..f32b458e93285adc 100644
--- a/arch/arm/boot/dts/r8a7794.dtsi
+++ b/arch/arm/boot/dts/r8a7794.dtsi
@@ -596,6 +596,14 @@
icram1: sram@e63c {
compatible = "mmio-sram";
reg = <0 0xe63c 0 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0 0xe63c 0x1000>;
+
+ smp-sram@0 {
+ compatible = "renesas,smp-sram";
+ reg = <0 0x10>;
+ };
};
ether: ethernet@ee70 {
--
2.7.4
[PATCH 8/9] ARM: dts: r8a7793: Reserve SRAM for the SMP jump stub
Reserve SRAM for the jump stub for CPU core bringup.
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7793.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7793.dtsi b/arch/arm/boot/dts/r8a7793.dtsi
index bc6a44272f555215..497716b6fbe24164 100644
--- a/arch/arm/boot/dts/r8a7793.dtsi
+++ b/arch/arm/boot/dts/r8a7793.dtsi
@@ -856,6 +856,14 @@
icram1: sram@e63c {
compatible = "mmio-sram";
reg = <0 0xe63c 0 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0 0xe63c 0x1000>;
+
+ smp-sram@0 {
+ compatible = "renesas,smp-sram";
+ reg = <0 0x10>;
+ };
};
ether: ethernet@ee70 {
--
2.7.4
[PATCH 0/9] ARM: renesas: Use SMP jump stub SRAM region from DT
Hi Magnus, Mark, Rob, Simon,
The R-Car Gen2 platform code for CPU core bringup needs to copy a jump
stub to on-SoC SRAM. Currently it uses a hardcoded address pointing to
ICRAM1.
This patch series adds support to specify this region from DT. It
consists of 3 parts:
- DT binding documentation for reserving SRAM for the jump stub,
- A platform code update to retrieve the information from DT, if
present (of course backwards-compatibility with old DTBs is
preserved),
- DT updates to reserve an SRAM region in DT on all R-Car Gen2 and
RZ/G1 SoCs.
The DT patches in this series depend on "[PATCH 0/7] ARM: dts: renesas:
Add Inter Connect RAM".
Note that the current jump stub in Linux is 12 bytes long. The patches
reserve 16 bytes of SRAM. Should this be increased? The mapping
granularity is PAGE_SIZE anyway.
Thanks for your comments!
Geert Uytterhoeven (9):
dt-bindings: sram: Document renesas,smp-sram
ARM: shmobile: rcar-gen2: Obtain jump stub region from DT
ARM: dts: r8a7743: Reserve SRAM for the SMP jump stub
ARM: dts: r8a7745: Reserve SRAM for the SMP jump stub
ARM: dts: r8a7790: Reserve SRAM for the SMP jump stub
ARM: dts: r8a7791: Reserve SRAM for the SMP jump stub
ARM: dts: r8a7792: Reserve SRAM for the SMP jump stub
ARM: dts: r8a7793: Reserve SRAM for the SMP jump stub
ARM: dts: r8a7794: Reserve SRAM for the SMP jump stub
.../devicetree/bindings/sram/renesas,smp-sram.txt | 27 ++
arch/arm/boot/dts/r8a7743.dtsi | 8 ++
arch/arm/boot/dts/r8a7745.dtsi | 8 ++
arch/arm/boot/dts/r8a7790.dtsi | 8 ++
arch/arm/boot/dts/r8a7791.dtsi | 8 ++
arch/arm/boot/dts/r8a7792.dtsi | 8 ++
arch/arm/boot/dts/r8a7793.dtsi | 8 ++
arch/arm/boot/dts/r8a7794.dtsi | 8 ++
arch/arm/mach-shmobile/pm-rcar-gen2.c | 33 --
9 files changed, 113 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/sram/renesas,smp-sram.txt
--
2.7.4
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 7/9] ARM: dts: r8a7792: Reserve SRAM for the SMP jump stub
Reserve SRAM for the jump stub for CPU core bringup.
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7792.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7792.dtsi b/arch/arm/boot/dts/r8a7792.dtsi
index 136a86ac64974adb..2623f39bed2b73bb 100644
--- a/arch/arm/boot/dts/r8a7792.dtsi
+++ b/arch/arm/boot/dts/r8a7792.dtsi
@@ -473,6 +473,14 @@
icram1: sram@e63c {
compatible = "mmio-sram";
reg = <0 0xe63c 0 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0 0xe63c 0x1000>;
+
+ smp-sram@0 {
+ compatible = "renesas,smp-sram";
+ reg = <0 0x10>;
+ };
};
sdhi0: sd@ee10 {
--
2.7.4
[PATCH 6/9] ARM: dts: r8a7791: Reserve SRAM for the SMP jump stub
Reserve SRAM for the jump stub for CPU core bringup.
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7791.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index f4748a9cb0375e4d..e135da440eed7b94 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -898,6 +898,14 @@
icram1: sram@e63c {
compatible = "mmio-sram";
reg = <0 0xe63c 0 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0 0xe63c 0x1000>;
+
+ smp-sram@0 {
+ compatible = "renesas,smp-sram";
+ reg = <0 0x10>;
+ };
};
ether: ethernet@ee70 {
--
2.7.4
[PATCH 4/9] ARM: dts: r8a7745: Reserve SRAM for the SMP jump stub
Reserve SRAM for the jump stub for CPU core bringup.
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7745.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7745.dtsi b/arch/arm/boot/dts/r8a7745.dtsi
index 88cf92bcd2f9b4fc..354534cdc5889cbc 100644
--- a/arch/arm/boot/dts/r8a7745.dtsi
+++ b/arch/arm/boot/dts/r8a7745.dtsi
@@ -481,6 +481,14 @@
icram1: sram@e63c {
compatible = "mmio-sram";
reg = <0 0xe63c 0 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0 0xe63c 0x1000>;
+
+ smp-sram@0 {
+ compatible = "renesas,smp-sram";
+ reg = <0 0x10>;
+ };
};
ether: ethernet@ee70 {
--
2.7.4
[PATCH 5/9] ARM: dts: r8a7790: Reserve SRAM for the SMP jump stub
Reserve SRAM for the jump stub for CPU core bringup.
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7790.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index 4ee34995573cf505..ff986df23b62135f 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -838,6 +838,14 @@
icram1: sram@e63c {
compatible = "mmio-sram";
reg = <0 0xe63c 0 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0 0xe63c 0x1000>;
+
+ smp-sram@0 {
+ compatible = "renesas,smp-sram";
+ reg = <0 0x10>;
+ };
};
ether: ethernet@ee70 {
--
2.7.4
[PATCH 1/9] dt-bindings: sram: Document renesas,smp-sram
Document reserved SRAM for the SMP jump stub on Renesas R-Car Gen2 and
RZ/G1 SoCs.
Signed-off-by: Geert Uytterhoeven
---
.../devicetree/bindings/sram/renesas,smp-sram.txt | 27 ++
1 file changed, 27 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sram/renesas,smp-sram.txt
diff --git a/Documentation/devicetree/bindings/sram/renesas,smp-sram.txt
b/Documentation/devicetree/bindings/sram/renesas,smp-sram.txt
new file mode 100644
index ..712d05e3e15eeb65
--- /dev/null
+++ b/Documentation/devicetree/bindings/sram/renesas,smp-sram.txt
@@ -0,0 +1,27 @@
+* Renesas SMP SRAM
+
+Renesas R-Car Gen2 and RZ/G1 SoCs need a small piece of SRAM for the jump stub
+for secondary CPU bringup and CPU hotplug.
+This memory is reserved by adding a child node to a "mmio-sram" node, cfr.
+Documentation/devicetree/bindings/sram/sram.txt.
+
+Required child node properties:
+ - compatible: Must be "renesas,smp-sram",
+ - reg: Address and length of the reserved SRAM.
+The full physical (bus) address must be aligned to a 256 KiB boundary.
+
+
+Example:
+
+ icram1: sram@e63c {
+ compatible = "mmio-sram";
+ reg = <0 0xe63c 0 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0 0xe63c 0x1000>;
+
+ smp-sram@0 {
+ compatible = "renesas,smp-sram";
+ reg = <0 0x10>;
+ };
+ };
--
2.7.4
[PATCH] ARM: dts: r8a7743: Add GPIO support
Describe GPIO blocks in the R8A7743 device tree.
Signed-off-by: Biju Das
Reviewed-by: Chris Paterson
---
This patch is been tested against linux-next tag next-20170704.
It depends upon the patch
1)https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg15744.html
2)https://www.spinics.net/lists/arm-kernel/msg577301.html
arch/arm/boot/dts/r8a7743.dtsi | 120 +
1 file changed, 120 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7743.dtsi b/arch/arm/boot/dts/r8a7743.dtsi
index 8d6b140..8380859 100644
--- a/arch/arm/boot/dts/r8a7743.dtsi
+++ b/arch/arm/boot/dts/r8a7743.dtsi
@@ -65,6 +65,126 @@
resets = <&cpg 408>;
};
+ gpio0: gpio@e605 {
+ compatible = "renesas,gpio-r8a7743",
+"renesas,gpio-rcar";
+ reg = <0 0xe605 0 0x50>;
+ interrupts = ;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 0 32>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 912>;
+ power-domains = <&sysc R8A7743_PD_ALWAYS_ON>;
+ resets = <&cpg 912>;
+ };
+
+ gpio1: gpio@e6051000 {
+ compatible = "renesas,gpio-r8a7743",
+"renesas,gpio-rcar";
+ reg = <0 0xe6051000 0 0x50>;
+ interrupts = ;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 32 26>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 911>;
+ power-domains = <&sysc R8A7743_PD_ALWAYS_ON>;
+ resets = <&cpg 911>;
+ };
+
+ gpio2: gpio@e6052000 {
+ compatible = "renesas,gpio-r8a7743",
+"renesas,gpio-rcar";
+ reg = <0 0xe6052000 0 0x50>;
+ interrupts = ;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 64 32>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 910>;
+ power-domains = <&sysc R8A7743_PD_ALWAYS_ON>;
+ resets = <&cpg 910>;
+ };
+
+ gpio3: gpio@e6053000 {
+ compatible = "renesas,gpio-r8a7743",
+"renesas,gpio-rcar";
+ reg = <0 0xe6053000 0 0x50>;
+ interrupts = ;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 96 32>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 909>;
+ power-domains = <&sysc R8A7743_PD_ALWAYS_ON>;
+ resets = <&cpg 909>;
+ };
+
+ gpio4: gpio@e6054000 {
+ compatible = "renesas,gpio-r8a7743",
+"renesas,gpio-rcar";
+ reg = <0 0xe6054000 0 0x50>;
+ interrupts = ;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 128 32>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 908>;
+ power-domains = <&sysc R8A7743_PD_ALWAYS_ON>;
+ resets = <&cpg 908>;
+ };
+
+ gpio5: gpio@e6055000 {
+ compatible = "renesas,gpio-r8a7743",
+"renesas,gpio-rcar";
+ reg = <0 0xe6055000 0 0x50>;
+ interrupts = ;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 160 32>;
+ #interrupt-cells = <2>;
+ interrup
Re: [PATCH 0/9] ARM: renesas: Use SMP jump stub SRAM region from DT
On Tue, Jul 4, 2017 at 5:41 PM, Geert Uytterhoeven
wrote:
> The R-Car Gen2 platform code for CPU core bringup needs to copy a jump
> stub to on-SoC SRAM. Currently it uses a hardcoded address pointing to
> ICRAM1.
>
> This patch series adds support to specify this region from DT. It
> consists of 3 parts:
> - DT binding documentation for reserving SRAM for the jump stub,
> - A platform code update to retrieve the information from DT, if
> present (of course backwards-compatibility with old DTBs is
> preserved),
> - DT updates to reserve an SRAM region in DT on all R-Car Gen2 and
> RZ/G1 SoCs.
>
> The DT patches in this series depend on "[PATCH 0/7] ARM: dts: renesas:
> Add Inter Connect RAM".
>
> Note that the current jump stub in Linux is 12 bytes long. The patches
> reserve 16 bytes of SRAM. Should this be increased? The mapping
> granularity is PAGE_SIZE anyway.
>
> Thanks for your comments!
>
> Geert Uytterhoeven (9):
> dt-bindings: sram: Document renesas,smp-sram
> ARM: shmobile: rcar-gen2: Obtain jump stub region from DT
> ARM: dts: r8a7743: Reserve SRAM for the SMP jump stub
> ARM: dts: r8a7745: Reserve SRAM for the SMP jump stub
> ARM: dts: r8a7790: Reserve SRAM for the SMP jump stub
> ARM: dts: r8a7791: Reserve SRAM for the SMP jump stub
> ARM: dts: r8a7792: Reserve SRAM for the SMP jump stub
> ARM: dts: r8a7793: Reserve SRAM for the SMP jump stub
> ARM: dts: r8a7794: Reserve SRAM for the SMP jump stub
Forgot to mention: this has been tested on r8a7790/lager, r8a7791/koelsch,
r8a7792/blanche, r8a7793/gose, and r8a7794/alt (r8a7794 needs an
unrelated fix to enable SMP).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 2/2] ARM: dts: r8a7794: Add SMP support
From: Sergei Shtylyov
Add the device tree node for the Advanced Power Management Unit (APMU).
Use the "enable-method" prop to point out that the APMU should be used
for the SMP support.
Signed-off-by: Sergei Shtylyov
Signed-off-by: Geert Uytterhoeven
---
arch/arm/boot/dts/r8a7794.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi
index f32b458e93285adc..8afd04339c84c65e 100644
--- a/arch/arm/boot/dts/r8a7794.dtsi
+++ b/arch/arm/boot/dts/r8a7794.dtsi
@@ -37,6 +37,7 @@
cpus {
#address-cells = <1>;
#size-cells = <0>;
+ enable-method = "renesas,apmu";
cpu0: cpu@0 {
device_type = "cpu";
@@ -65,6 +66,12 @@
};
};
+ apmu@e6151000 {
+ compatible = "renesas,r8a7794-apmu", "renesas,apmu";
+ reg = <0 0xe6151000 0 0x188>;
+ cpus = <&cpu0 &cpu1>;
+ };
+
gic: interrupt-controller@f1001000 {
compatible = "arm,gic-400";
#interrupt-cells = <3>;
--
2.7.4
[PATCH 0/2] ARM: renesas: Enable SMP on R-Car E2
Hi Magnus, Mar[ck], Sergei, Simon,
This patch series enables SMP on R-Car E2 (r8a7794).
- The first patch initializes CNTVOFF for secondary CPU cores, like is
already done for the boot CPU core. Without this, the ARM arch
timer doesn't work on secondary CPU cores.
- The second patch adds the required infrastructure (APMU device node
and corresponding enable-method) to DT.
This must not be applied on a branch that doesn't have the first
patch!
According to the original comments, the CNTVOFF initialization is needed
on Cortex-A7 only, hence affects R-Car E2, which has a dual Cortex-A7
cluster.
Apparently it's not needed on other R-Car Gen2 members that have (only)
Cortex-A15 clusters.
Perhaps it's also needed on R-Car H2, which has a big.LITTLE
quad Cortex-A15 + quad Cortex-A7 configuration, when running Linux on
the Cortex-A7 cores? How can I test that?
Thanks for your comments!
Geert Uytterhoeven (1):
ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs
Sergei Shtylyov (1):
ARM: dts: r8a7794: Add SMP support
arch/arm/boot/dts/r8a7794.dtsi| 7 +++
arch/arm/mach-shmobile/Makefile | 1 +
arch/arm/mach-shmobile/common.h | 1 +
arch/arm/mach-shmobile/headsmp-apmu.S | 38 +++
arch/arm/mach-shmobile/platsmp-apmu.c | 2 +-
5 files changed, 48 insertions(+), 1 deletion(-)
create mode 100644 arch/arm/mach-shmobile/headsmp-apmu.S
--
2.7.4
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 1/2] ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs
On Cortex-A7, the arch timer CNTVOFF register is uninitialized.
Hence when enabling SMP on r8a7794, the kernel log is spammed with:
WARNING: Underflow in clocksource 'arch_sys_counter' observed, time update
ignored.
Please report this, consider using a different clocksource, if
possible.
Your kernel is probably still fine.
To fix this, wrap the standard secondary_startup routine inside a
routine which initializes CNTVOFF when running on a Cortex-A7.
As the only possibilities are Cortex-A7 or Cortex-A15, checking the low
nibble of the Primary Part Number is sufficient.
The initialization is identical to what is already done for the boot CPU
since commit 9ce3fa6816c2fb59 ("ARM: shmobile: rcar-gen2: Add CA7
arch_timer initialization for r8a7794").
Based on patches by Hisashi Nakamura in the BSP.
Signed-off-by: Geert Uytterhoeven
---
arch/arm/mach-shmobile/Makefile | 1 +
arch/arm/mach-shmobile/common.h | 1 +
arch/arm/mach-shmobile/headsmp-apmu.S | 38 +++
arch/arm/mach-shmobile/platsmp-apmu.c | 2 +-
4 files changed, 41 insertions(+), 1 deletion(-)
create mode 100644 arch/arm/mach-shmobile/headsmp-apmu.S
diff --git a/arch/arm/mach-shmobile/Makefile b/arch/arm/mach-shmobile/Makefile
index 64611a1b4276517b..de735444e9f715fd 100644
--- a/arch/arm/mach-shmobile/Makefile
+++ b/arch/arm/mach-shmobile/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_ARCH_R8A7793)+= regulator-quirk-rcar-gen2.o
# SMP objects
smp-y := $(cpu-y)
+smp-$(CONFIG_ARCH_RCAR_GEN2) += headsmp-apmu.o
smp-$(CONFIG_ARCH_SH73A0) += smp-sh73a0.o headsmp-scu.o platsmp-scu.o
smp-$(CONFIG_ARCH_R8A7779) += smp-r8a7779.o headsmp-scu.o platsmp-scu.o
smp-$(CONFIG_ARCH_R8A7790) += smp-r8a7790.o
diff --git a/arch/arm/mach-shmobile/common.h b/arch/arm/mach-shmobile/common.h
index 1a8f7b3ab449db56..6792a07bce761aab 100644
--- a/arch/arm/mach-shmobile/common.h
+++ b/arch/arm/mach-shmobile/common.h
@@ -11,6 +11,7 @@ extern void shmobile_smp_hook(unsigned int cpu, unsigned long
fn,
unsigned long arg);
extern bool shmobile_smp_cpu_can_disable(unsigned int cpu);
extern bool shmobile_smp_init_fallback_ops(void);
+extern void shmobile_boot_apmu(void);
extern void shmobile_boot_scu(void);
extern void shmobile_smp_scu_prepare_cpus(phys_addr_t scu_base_phys,
unsigned int max_cpus);
diff --git a/arch/arm/mach-shmobile/headsmp-apmu.S
b/arch/arm/mach-shmobile/headsmp-apmu.S
new file mode 100644
index ..52516d81ce98384c
--- /dev/null
+++ b/arch/arm/mach-shmobile/headsmp-apmu.S
@@ -0,0 +1,38 @@
+/*
+ * SMP support for APMU based systems
+ *
+ * Copyright (C) 2014 Renesas Electronics Corporation
+ *
+ * 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.
+ */
+
+#include
+
+ENTRY(shmobile_boot_apmu)
+ mrc p15, 0, r0, c0, c0, 0 /* Get Main ID */
+ ubfxr1, r0, #4, #4 /* r1=Lo 4bit of Primary Part */
+ cmp r1, #0x7/* 0x7 = CA7, 0xf = CA15 */
+ bne 1f
+ /*
+* CA7 setup
+* CNTVOFF has to be initialized either from non-secure Hypervisor
+* mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
+* then it should be handled by the secure code
+*/
+ cps 0x16
+ mrc p15, 0, r1, c1, c1, 0 /* get Secure Config */
+ orr r0, r1, #1
+ mcr p15, 0, r0, c1, c1, 0 /* Set Non Secure bit */
+ isb
+ mov r0, #0
+ mcrrp15, 4, r0, r0, c14 /* CNTVOFF = 0 */
+ isb
+ mcr p15, 0, r1, c1, c1, 0 /* Set Secure bit */
+ isb
+ cps 0x13
+1:
+
+ b secondary_startup
+ENDPROC(shmobile_boot_apmu)
diff --git a/arch/arm/mach-shmobile/platsmp-apmu.c
b/arch/arm/mach-shmobile/platsmp-apmu.c
index 3ca2c13346f0cbc3..4422b615a6ee6045 100644
--- a/arch/arm/mach-shmobile/platsmp-apmu.c
+++ b/arch/arm/mach-shmobile/platsmp-apmu.c
@@ -204,7 +204,7 @@ void __init shmobile_smp_apmu_prepare_cpus(unsigned int
max_cpus,
int shmobile_smp_apmu_boot_secondary(unsigned int cpu, struct task_struct
*idle)
{
/* For this particular CPU register boot vector */
- shmobile_smp_hook(cpu, __pa_symbol(secondary_startup), 0);
+ shmobile_smp_hook(cpu, __pa_symbol(shmobile_boot_apmu), 0);
return apmu_wrap(cpu, apmu_power_on);
}
--
2.7.4
Re: [PATCH 1/2] ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs
Hi Geert,
On 04/07/17 18:02, Geert Uytterhoeven wrote:
> On Cortex-A7, the arch timer CNTVOFF register is uninitialized.
> Hence when enabling SMP on r8a7794, the kernel log is spammed with:
>
> WARNING: Underflow in clocksource 'arch_sys_counter' observed, time
> update ignored.
>Please report this, consider using a different clocksource, if
> possible.
>Your kernel is probably still fine.
>
> To fix this, wrap the standard secondary_startup routine inside a
> routine which initializes CNTVOFF when running on a Cortex-A7.
> As the only possibilities are Cortex-A7 or Cortex-A15, checking the low
> nibble of the Primary Part Number is sufficient.
>
> The initialization is identical to what is already done for the boot CPU
> since commit 9ce3fa6816c2fb59 ("ARM: shmobile: rcar-gen2: Add CA7
> arch_timer initialization for r8a7794").
Humfff... Pretty horrible. Comments below.
>
> Based on patches by Hisashi Nakamura in the BSP.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> arch/arm/mach-shmobile/Makefile | 1 +
> arch/arm/mach-shmobile/common.h | 1 +
> arch/arm/mach-shmobile/headsmp-apmu.S | 38
> +++
> arch/arm/mach-shmobile/platsmp-apmu.c | 2 +-
> 4 files changed, 41 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/mach-shmobile/headsmp-apmu.S
>
> diff --git a/arch/arm/mach-shmobile/Makefile b/arch/arm/mach-shmobile/Makefile
> index 64611a1b4276517b..de735444e9f715fd 100644
> --- a/arch/arm/mach-shmobile/Makefile
> +++ b/arch/arm/mach-shmobile/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_ARCH_R8A7793) += regulator-quirk-rcar-gen2.o
>
> # SMP objects
> smp-y:= $(cpu-y)
> +smp-$(CONFIG_ARCH_RCAR_GEN2) += headsmp-apmu.o
> smp-$(CONFIG_ARCH_SH73A0)+= smp-sh73a0.o headsmp-scu.o platsmp-scu.o
> smp-$(CONFIG_ARCH_R8A7779) += smp-r8a7779.o headsmp-scu.o platsmp-scu.o
> smp-$(CONFIG_ARCH_R8A7790) += smp-r8a7790.o
> diff --git a/arch/arm/mach-shmobile/common.h b/arch/arm/mach-shmobile/common.h
> index 1a8f7b3ab449db56..6792a07bce761aab 100644
> --- a/arch/arm/mach-shmobile/common.h
> +++ b/arch/arm/mach-shmobile/common.h
> @@ -11,6 +11,7 @@ extern void shmobile_smp_hook(unsigned int cpu, unsigned
> long fn,
> unsigned long arg);
> extern bool shmobile_smp_cpu_can_disable(unsigned int cpu);
> extern bool shmobile_smp_init_fallback_ops(void);
> +extern void shmobile_boot_apmu(void);
> extern void shmobile_boot_scu(void);
> extern void shmobile_smp_scu_prepare_cpus(phys_addr_t scu_base_phys,
> unsigned int max_cpus);
> diff --git a/arch/arm/mach-shmobile/headsmp-apmu.S
> b/arch/arm/mach-shmobile/headsmp-apmu.S
> new file mode 100644
> index ..52516d81ce98384c
> --- /dev/null
> +++ b/arch/arm/mach-shmobile/headsmp-apmu.S
> @@ -0,0 +1,38 @@
> +/*
> + * SMP support for APMU based systems
> + *
> + * Copyright (C) 2014 Renesas Electronics Corporation
> + *
> + * 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.
> + */
> +
> +#include
> +
> +ENTRY(shmobile_boot_apmu)
> + mrc p15, 0, r0, c0, c0, 0 /* Get Main ID */
> + ubfxr1, r0, #4, #4 /* r1=Lo 4bit of Primary Part */
> + cmp r1, #0x7/* 0x7 = CA7, 0xf = CA15 */
> + bne 1f
Why don't you deal with the A15 parts as well? And TBH, you'd be better
off checking ID_PFR1 for both Generic Timers and Virtualization Extensions.
> + /*
> + * CA7 setup
> + * CNTVOFF has to be initialized either from non-secure Hypervisor
> + * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> + * then it should be handled by the secure code
> + */
> + cps 0x16
It'd be worth adding a MONITOR_MODE macro instead of this raw value.
> + mrc p15, 0, r1, c1, c1, 0 /* get Secure Config */
> + orr r0, r1, #1
> + mcr p15, 0, r0, c1, c1, 0 /* Set Non Secure bit */
> + isb
> + mov r0, #0
> + mcrrp15, 4, r0, r0, c14 /* CNTVOFF = 0 */
> + isb
> + mcr p15, 0, r1, c1, c1, 0 /* Set Secure bit */
> + isb
> + cps 0x13
and use SVC_MODE here.
> +1:
> +
> + b secondary_startup
> +ENDPROC(shmobile_boot_apmu)
> diff --git a/arch/arm/mach-shmobile/platsmp-apmu.c
> b/arch/arm/mach-shmobile/platsmp-apmu.c
> index 3ca2c13346f0cbc3..4422b615a6ee6045 100644
> --- a/arch/arm/mach-shmobile/platsmp-apmu.c
> +++ b/arch/arm/mach-shmobile/platsmp-apmu.c
> @@ -204,7 +204,7 @@ void __init shmobile_smp_apmu_prepare_cpus(unsigned int
> max_cpus,
> int shmobile_smp_apmu_boot_secondary(unsigned int cpu, struct task_struct
> *idle)
> {
> /* For this particular CPU register boot vector */
> - shmobil
[PATCH/RFC 0/2] ARM: renesas: rcar-gen2: arch_timer cleanups
Hi Simon, Magnus, Sergei,
This RFC patch series contains two small fixes for the ARM architecture
timer initialization in platform code for Renesas R-Car Gen2 and RZ/G1
SoCs.
- The first patch skips an unneeded initialization on R-Car V2h.
This is marked RFC as the check for Cortex-A7 may have to be
changed, depending on the discussion sparked by "[PATCH 0/2] ARM:
renesas: Enable SMP on R-Car E2".
- The second patch corrects the frequency of the architecture timer on
RZ/G1E.
This is marked RFC as I don't have access to hardware to test.
Thanks for your comments!
Geert Uytterhoeven (2):
ARM: shmobile: rcar-gen2: Separate arch timer init and parent handling
ARM: shmobile: rcar-gen2: Correct arch timer frequency on RZ/G1E
arch/arm/mach-shmobile/setup-rcar-gen2.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)
--
2.7.4
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH/RFC 1/2] ARM: shmobile: rcar-gen2: Separate arch timer init and parent handling
The check for R-Car E2 (which was later extended to R-Car V2H), served
two purposes:
1. Use the correct parent clock for the arch timer,
2. Initialize the arch timer.
However, the second part shouldn't be done on V2H, as it only applies to
Cortex-A7 CPU cores, while V2H has a dual Cortex-A15.
Hence split off the arch timer initialization, and make it run on
Cortex-A7 only.
While at it, add some documentation to the inline asm.
Fixes: 2477a356dd245bbb ("ARM: shmobile: rcar-gen2: Correct arch timer
frequency on R-Car V2H")
Signed-off-by: Geert Uytterhoeven
---
arch/arm/mach-shmobile/setup-rcar-gen2.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c
b/arch/arm/mach-shmobile/setup-rcar-gen2.c
index 52d466b759730d74..caa866d406e50795 100644
--- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include "common.h"
#include "rcar-gen2.h"
@@ -58,27 +59,31 @@ void __init rcar_gen2_timer_init(void)
void __iomem *base;
u32 freq;
- if (of_machine_is_compatible("renesas,r8a7792") ||
- of_machine_is_compatible("renesas,r8a7794")) {
- freq = 26000 / 8; /* ZS / 8 */
- /* CNTVOFF has to be initialized either from non-secure
+ if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A7) {
+ /*
+* CNTVOFF has to be initialized either from non-secure
* Hypervisor mode or secure Monitor mode with SCR.NS==1.
* If TrustZone is enabled then it should be handled by the
* secure code.
*/
asm volatile(
" cps 0x16\n"
- " mrc p15, 0, r1, c1, c1, 0\n"
+ " mrc p15, 0, r1, c1, c1, 0\n" /* Get Secure Config */
" orr r0, r1, #1\n"
- " mcr p15, 0, r0, c1, c1, 0\n"
+ " mcr p15, 0, r0, c1, c1, 0\n" /* Set Non Secure bit
*/
" isb\n"
" mov r0, #0\n"
- " mcrrp15, 4, r0, r0, c14\n"
+ " mcrrp15, 4, r0, r0, c14\n" /* CNTVOFF = 0 */
" isb\n"
- " mcr p15, 0, r1, c1, c1, 0\n"
+ " mcr p15, 0, r1, c1, c1, 0\n" /* Set Secure bit */
" isb\n"
" cps 0x13\n"
: : : "r0", "r1");
+ }
+
+ if (of_machine_is_compatible("renesas,r8a7792") ||
+ of_machine_is_compatible("renesas,r8a7794")) {
+ freq = 26000 / 8; /* ZS / 8 */
} else {
/* At Linux boot time the r8a7790 arch timer comes up
* with the counter disabled. Moreover, it may also report
--
2.7.4
[PATCH/RFC 2/2] ARM: shmobile: rcar-gen2: Correct arch timer frequency on RZ/G1E
According to the datasheet, the frequency of the ARM architecture timer
on RZ/G1E depends on the frequency of the ZS clock, just like on R-Car
E2 and V2H.
Signed-off-by: Geert Uytterhoeven
---
Untested due to lack of hardware.
---
arch/arm/mach-shmobile/setup-rcar-gen2.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c
b/arch/arm/mach-shmobile/setup-rcar-gen2.c
index caa866d406e50795..c88c4aaddf97eb06 100644
--- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
@@ -81,7 +81,8 @@ void __init rcar_gen2_timer_init(void)
: : : "r0", "r1");
}
- if (of_machine_is_compatible("renesas,r8a7792") ||
+ if (of_machine_is_compatible("renesas,r8a7745") ||
+ of_machine_is_compatible("renesas,r8a7792") ||
of_machine_is_compatible("renesas,r8a7794")) {
freq = 26000 / 8; /* ZS / 8 */
} else {
--
2.7.4
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
> Hi Krzysztof, Rafael,
>
> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski wrote:
> > genpd_syscore_switch() had two problems:
> > 1. It silently assumed that device, it is being called for, belongs to
> >generic power domain and used container_of() on its power domain
> >pointer. Such assumption might not be true always.
> >
> > 2. It iterated over list of generic power domains without holding
> >gpd_list_lock mutex thus list could have been modified in the same
> >time.
> >
> > Usage of genpd_lookup_dev() solves both problems as it is safe a call
> > for non-generic power domains and uses mutex when iterating.
> >
> > Reported-by: Ulf Hansson
> > Signed-off-by: Krzysztof Kozlowski
> > Acked-by: Ulf Hansson
>
> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
> request "[GIT PULL] Power management updates for v4.13-rc1".
>
> > Not tested on real hardware.
>
> This causes the following BUG during s2ram on all my Renesas arm32 boards,
> where the system timer is an IRQ safe device:
>
> PM: Syncing filesystems ... done.
> PM: Preparing system for sleep (mem)
> Freezing user space processes ... (elapsed 0.001 seconds) done.
> OOM killer disabled.
> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> PM: Suspending system (mem)
> PM: suspend of devices complete after 122.841 msecs
> PM: suspend devices took 0.130 seconds
> PM: late suspend of devices complete after 2.682 msecs
> PM: noirq suspend of devices complete after 29.951 msecs
> Disabling non-boot CPUs ...
> BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:238
Hi,
Thanks for report!
Damn it, although I couldn't find this in the code, but I was fearing
that this ends up in atomic section. That would kind of explain why
mutex was not there [1].
Anyway, the buggy code was there already. Instead of "sleeping in atomic
section" there was no locking at all... In this context this was
probably safe because it was executed *after* disabling non-boot CPUs
but then the function cannot be called in other contexts.
I am not sure I will be capable of developing the proper fix as I do not
have the hardware and I do not know all stuff happening in sh suspend.
Probably reverting this and living with non-locked path would be the
safest choice.
[1] https://patchwork.kernel.org/patch/9778903/
Best regards,
Krzysztof
> in_atomic(): 0, irqs_disabled(): 128, pid: 1730, name: s2ram
> CPU: 0 PID: 1730 Comm: s2ram Not tainted
> 4.12.0-koelsch-07061-g810fee9afeba15ef #3592
> Hardware name: Generic R8A7791 (Flattened Device Tree)
> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> [] (show_stack) from [] (dump_stack+0x7c/0x9c)
> [] (dump_stack) from [] (___might_sleep+0x124/0x160)
> [] (___might_sleep) from [] (mutex_lock+0x18/0x60)
> [] (mutex_lock) from [] (genpd_lookup_dev+0x38/0x94)
> [] (genpd_lookup_dev) from []
> (pm_genpd_syscore_poweroff+0x8/0x2c)
> [] (pm_genpd_syscore_poweroff) from []
> (sh_cmt_clock_event_suspend+0x18/0x28)
> [] (sh_cmt_clock_event_suspend) from []
> (clockevents_suspend+0x40/0x54)
> [] (clockevents_suspend) from []
> (timekeeping_suspend+0x23c/0x278)
> [] (timekeeping_suspend) from []
> (syscore_suspend+0x88/0x138)
> [] (syscore_suspend) from []
> (suspend_devices_and_enter+0x308/0x4fc)
> [] (suspend_devices_and_enter) from []
> (pm_suspend+0x228/0x280)
> [] (pm_suspend) from [] (state_store+0xac/0xcc)
> [] (state_store) from [] (kernfs_fop_write+0x170/0x1b0)
> [] (kernfs_fop_write) from [] (__vfs_write+0x20/0x108)
> [] (__vfs_write) from [] (vfs_write+0xb8/0x144)
> [] (vfs_write) from [] (SyS_write+0x40/0x80)
> [] (SyS_write) from [] (ret_fast_syscall+0x0/0x34)
>
> Reverting the commit fixes the issue.
>
> > ---
> > drivers/base/power/domain.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 01e31d9f6c94..d31a4434b8b3 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -1098,8 +1098,8 @@ static void genpd_syscore_switch(struct device *dev,
> > bool suspend)
> > {
> > struct generic_pm_domain *genpd;
> >
> > - genpd = dev_to_genpd(dev);
> > - if (!pm_genpd_present(genpd))
> > + genpd = genpd_lookup_dev(dev);
> > + if (!genpd)
> > return;
> >
> > if (suspend) {
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
Hi Krzysztof,
On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski wrote:
> On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
>> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski wrote:
>> > genpd_syscore_switch() had two problems:
>> > 1. It silently assumed that device, it is being called for, belongs to
>> >generic power domain and used container_of() on its power domain
>> >pointer. Such assumption might not be true always.
>> >
>> > 2. It iterated over list of generic power domains without holding
>> >gpd_list_lock mutex thus list could have been modified in the same
>> >time.
>> >
>> > Usage of genpd_lookup_dev() solves both problems as it is safe a call
>> > for non-generic power domains and uses mutex when iterating.
>> >
>> > Reported-by: Ulf Hansson
>> > Signed-off-by: Krzysztof Kozlowski
>> > Acked-by: Ulf Hansson
>>
>> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
>> request "[GIT PULL] Power management updates for v4.13-rc1".
>>
>> > Not tested on real hardware.
>>
>> This causes the following BUG during s2ram on all my Renesas arm32 boards,
>> where the system timer is an IRQ safe device:
>>
>> PM: Syncing filesystems ... done.
>> PM: Preparing system for sleep (mem)
>> Freezing user space processes ... (elapsed 0.001 seconds) done.
>> OOM killer disabled.
>> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>> PM: Suspending system (mem)
>> PM: suspend of devices complete after 122.841 msecs
>> PM: suspend devices took 0.130 seconds
>> PM: late suspend of devices complete after 2.682 msecs
>> PM: noirq suspend of devices complete after 29.951 msecs
>> Disabling non-boot CPUs ...
>> BUG: sleeping function called from invalid context at
>> kernel/locking/mutex.c:238
>
> Thanks for report!
>
> Damn it, although I couldn't find this in the code, but I was fearing
> that this ends up in atomic section. That would kind of explain why
> mutex was not there [1].
>
> Anyway, the buggy code was there already. Instead of "sleeping in atomic
> section" there was no locking at all... In this context this was
> probably safe because it was executed *after* disabling non-boot CPUs
> but then the function cannot be called in other contexts.
>
> I am not sure I will be capable of developing the proper fix as I do not
> have the hardware and I do not know all stuff happening in sh suspend.
> Probably reverting this and living with non-locked path would be the
> safest choice.
>
> [1] https://patchwork.kernel.org/patch/9778903/
AFAIU, all syscore stuff runs in atomic context.
Don't worry, you're not the only one.
This bug report was almost 100% the same as an earlier one for a patch
from Ulf ;-)
(cfr. "[RESEND PATCH 0/2] PM / Domains: Fix asynchronous execution of
*noirq() callbacks")
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 1/2] ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs
Hi Marc,
On Tue, Jul 4, 2017 at 7:32 PM, Marc Zyngier wrote:
> On 04/07/17 18:02, Geert Uytterhoeven wrote:
>> On Cortex-A7, the arch timer CNTVOFF register is uninitialized.
>> Hence when enabling SMP on r8a7794, the kernel log is spammed with:
>>
>> WARNING: Underflow in clocksource 'arch_sys_counter' observed, time
>> update ignored.
>>Please report this, consider using a different clocksource, if
>> possible.
>>Your kernel is probably still fine.
>>
>> To fix this, wrap the standard secondary_startup routine inside a
>> routine which initializes CNTVOFF when running on a Cortex-A7.
>> As the only possibilities are Cortex-A7 or Cortex-A15, checking the low
>> nibble of the Primary Part Number is sufficient.
>>
>> The initialization is identical to what is already done for the boot CPU
>> since commit 9ce3fa6816c2fb59 ("ARM: shmobile: rcar-gen2: Add CA7
>> arch_timer initialization for r8a7794").
>
> Humfff... Pretty horrible. Comments below.
I know. I suppose this should be done by the firmware/boot loader?
But we have to live with firmware/boot loaders in the wild...
>> --- /dev/null
>> +++ b/arch/arm/mach-shmobile/headsmp-apmu.S
>> @@ -0,0 +1,38 @@
>> +/*
>> + * SMP support for APMU based systems
>> + *
>> + * Copyright (C) 2014 Renesas Electronics Corporation
>> + *
>> + * 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.
>> + */
>> +
>> +#include
>> +
>> +ENTRY(shmobile_boot_apmu)
>> + mrc p15, 0, r0, c0, c0, 0 /* Get Main ID */
>> + ubfxr1, r0, #4, #4 /* r1=Lo 4bit of Primary Part
>> */
>> + cmp r1, #0x7/* 0x7 = CA7, 0xf = CA15 */
>> + bne 1f
>
> Why don't you deal with the A15 parts as well? And TBH, you'd be better
> off checking ID_PFR1 for both Generic Timers and Virtualization Extensions.
Do we have to deal with the A15 parts here?
We're not having issues with the A15 parts, so that's why I left it out.
I'm trying to understand what's different between A15 and A7, and why A7
needs this code, while A15 apparently doesn't.
I'm not seeing any obvious differences in the U-Boot sources handling
e.g. r8a7791/koelsch (dual A15) vs. r8a7794/alt (dual A7).
> off checking ID_PFR1 for both Generic Timers and Virtualization Extensions.
Thank, I'll have a look at that...
>> + /*
>> + * CA7 setup
>> + * CNTVOFF has to be initialized either from non-secure Hypervisor
>> + * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
>> + * then it should be handled by the secure code
>> + */
>> + cps 0x16
>
> It'd be worth adding a MONITOR_MODE macro instead of this raw value.
>> + cps 0x13
>
> and use SVC_MODE here.
Thanks, I'll have a look at those, too.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Architecture Timer on R-Car Gen2
Hi Magnus, Mar[ck], Simon,
We're (finally) getting close to the point where Renesas R-Car Gen2
platform code no longer relies on hardcoded addresses.
The remaining parts are related to the ARM Architecture Timer / Generic
Timer / Generic Counter. More specifically the code in
rcar_gen2_timer_init() in arch/arm/mach-shmobile/setup-rcar-gen2.c,
quoted below. According to historical information, the issues being
solved here are (1) non-running timers, and (2) wrong frequencies,
leading to (m)sleep() not sleeping for the expected period.
But it's far from clear to me what's really going on...
> void __iomem *base;
> u32 freq;
>
> /*
>* Determine frequency. Depending on SoC-type, it's either
>* - Crystal / 2 => (usually) 10 MHz (calculated from extal in DT)
>* - ZS clock / 8 => (always) 32.5 MHz (hardcoded)
>*/
> freq = ...
>
> /* Remap "armgcnt address map" space */
> base = ioremap(0xe608, PAGE_SIZE);
I believe this region corresponds to the Generic Counter, which is not
the same, but related to the Architecture timer?
It seems no DT bindings exist for the Generic Counter?
(for a moment I thought this might correspond to "arm,armv7-timer-mem", but
I no longer believe that's true)
> /*
>* Update the timer if it is either not running, or is not at the
>* right frequency. The timer is only configurable in secure mode
>* so this avoids an abort if the loader started the timer and
>* entered the kernel in non-secure mode.
>*/
>
> if ((ioread32(base + CNTCR) & 1) == 0 ||
> ioread32(base + CNTFID0) != freq) {
On all SoC/board combos I checked (r8a7790/lager, r8a7791/koelsch,
r8a7791/porter, r8a7792/blanche, r8a7793/gose, r8a7794/alt), both
conditions are true, i.e.
- The timer (counter?) is not running,
- The configured frequency is 13 MHz, which is wrong.
> /* Update registers with correct frequency */
> iowrite32(freq, base + CNTFID0);
This doesn't seem to be needed?
With or without, both msleep() (kernel) and sleep() (userspace) sleep
for the expected amount of time.
> asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (freq));
This is an open-coded implementation of arch_timer_set_cntfrq(), which
is not provided by arch/arm/include/asm/arch_timer.h (should it?).
Without this, the arch_timer driver complains:
arch_timer: frequency not available
leading to failures (e.g. division by zero) later.
However, adding the proper "clock-frequency" property to the
"arm,armv7-timer" device node in DT fixes that.
Probably adding this property is a better way to work around broken(?)
firmware not setting CNTFRQ, than checking the SoC type and using
hardcoded values in platform code?
> /* make sure arch timer is started by setting bit 0 of CNTCR */
> iowrite32(1, base + CNTCR);
This is definitely needed. Else the timer doesn't run.
> }
>
> iounmap(base);
Thanks for your time!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote:
> Hi Krzysztof,
>
> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski wrote:
> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski
> >> wrote:
> >> > genpd_syscore_switch() had two problems:
> >> > 1. It silently assumed that device, it is being called for, belongs to
> >> >generic power domain and used container_of() on its power domain
> >> >pointer. Such assumption might not be true always.
> >> >
> >> > 2. It iterated over list of generic power domains without holding
> >> >gpd_list_lock mutex thus list could have been modified in the same
> >> >time.
> >> >
> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call
> >> > for non-generic power domains and uses mutex when iterating.
> >> >
> >> > Reported-by: Ulf Hansson
> >> > Signed-off-by: Krzysztof Kozlowski
> >> > Acked-by: Ulf Hansson
> >>
> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
> >> request "[GIT PULL] Power management updates for v4.13-rc1".
> >>
> >> > Not tested on real hardware.
> >>
> >> This causes the following BUG during s2ram on all my Renesas arm32 boards,
> >> where the system timer is an IRQ safe device:
> >>
> >> PM: Syncing filesystems ... done.
> >> PM: Preparing system for sleep (mem)
> >> Freezing user space processes ... (elapsed 0.001 seconds) done.
> >> OOM killer disabled.
> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> >> PM: Suspending system (mem)
> >> PM: suspend of devices complete after 122.841 msecs
> >> PM: suspend devices took 0.130 seconds
> >> PM: late suspend of devices complete after 2.682 msecs
> >> PM: noirq suspend of devices complete after 29.951 msecs
> >> Disabling non-boot CPUs ...
> >> BUG: sleeping function called from invalid context at
> >> kernel/locking/mutex.c:238
> >
> > Thanks for report!
> >
> > Damn it, although I couldn't find this in the code, but I was fearing
> > that this ends up in atomic section. That would kind of explain why
> > mutex was not there [1].
> >
> > Anyway, the buggy code was there already. Instead of "sleeping in atomic
> > section" there was no locking at all... In this context this was
> > probably safe because it was executed *after* disabling non-boot CPUs
> > but then the function cannot be called in other contexts.
> >
> > I am not sure I will be capable of developing the proper fix as I do not
> > have the hardware and I do not know all stuff happening in sh suspend.
> > Probably reverting this and living with non-locked path would be the
> > safest choice.
> >
> > [1] https://patchwork.kernel.org/patch/9778903/
>
> AFAIU, all syscore stuff runs in atomic context.
Indeed... The confusing part is that this code is syscore only from
the name, it is not hooked in to syscore_ops. Although going by call
chain (through sh clocksource drivers) we end up in
timekeeping_suspend() which is a syscore op.
I wonder whether it would be useful - after reverting my commit - to add
an assert (which is a stronger API requirement than only documentation "may
only be called during the system core (syscore) suspend") like:
WARN_ON(num_online_cpus() > 1));
as without mutexes this should not be executed with more than one online
CPU.
Best regards,
Krzysztof
>
> Don't worry, you're not the only one.
> This bug report was almost 100% the same as an earlier one for a patch
> from Ulf ;-)
> (cfr. "[RESEND PATCH 0/2] PM / Domains: Fix asynchronous execution of
> *noirq() callbacks")
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 4, 2017 at 8:36 PM, Krzysztof Kozlowski wrote: > On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote: >> Hi Krzysztof, >> >> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski wrote: >> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote: >> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski >> >> wrote: >> >> > genpd_syscore_switch() had two problems: >> >> > 1. It silently assumed that device, it is being called for, belongs to >> >> >generic power domain and used container_of() on its power domain >> >> >pointer. Such assumption might not be true always. >> >> > >> >> > 2. It iterated over list of generic power domains without holding >> >> >gpd_list_lock mutex thus list could have been modified in the same >> >> >time. >> >> > >> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call >> >> > for non-generic power domains and uses mutex when iterating. >> >> > >> >> > Reported-by: Ulf Hansson >> >> > Signed-off-by: Krzysztof Kozlowski >> >> > Acked-by: Ulf Hansson >> >> >> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull >> >> request "[GIT PULL] Power management updates for v4.13-rc1". >> >> >> >> > Not tested on real hardware. >> >> >> >> This causes the following BUG during s2ram on all my Renesas arm32 boards, >> >> where the system timer is an IRQ safe device: >> >> >> >> PM: Syncing filesystems ... done. >> >> PM: Preparing system for sleep (mem) >> >> Freezing user space processes ... (elapsed 0.001 seconds) done. >> >> OOM killer disabled. >> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. >> >> PM: Suspending system (mem) >> >> PM: suspend of devices complete after 122.841 msecs >> >> PM: suspend devices took 0.130 seconds >> >> PM: late suspend of devices complete after 2.682 msecs >> >> PM: noirq suspend of devices complete after 29.951 msecs >> >> Disabling non-boot CPUs ... >> >> BUG: sleeping function called from invalid context at >> >> kernel/locking/mutex.c:238 >> > >> > Thanks for report! >> > >> > Damn it, although I couldn't find this in the code, but I was fearing >> > that this ends up in atomic section. That would kind of explain why >> > mutex was not there [1]. >> > >> > Anyway, the buggy code was there already. Instead of "sleeping in atomic >> > section" there was no locking at all... In this context this was >> > probably safe because it was executed *after* disabling non-boot CPUs >> > but then the function cannot be called in other contexts. >> > >> > I am not sure I will be capable of developing the proper fix as I do not >> > have the hardware and I do not know all stuff happening in sh suspend. >> > Probably reverting this and living with non-locked path would be the >> > safest choice. >> > >> > [1] https://patchwork.kernel.org/patch/9778903/ >> >> AFAIU, all syscore stuff runs in atomic context. > > Indeed... The confusing part is that this code is syscore only from > the name, it is not hooked in to syscore_ops. Although going by call > chain (through sh clocksource drivers) we end up in > timekeeping_suspend() which is a syscore op. > > I wonder whether it would be useful - after reverting my commit - to add > an assert (which is a stronger API requirement than only documentation "may > only be called during the system core (syscore) suspend") like: > WARN_ON(num_online_cpus() > 1)); > as without mutexes this should not be executed with more than one online > CPU. Or maybe WARN_ON_ONCE(!in_atomic())? I'm queuing up a revert of the $subject commit. Thanks, Rafael
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 04, 2017 at 09:54:10PM +0200, Rafael J. Wysocki wrote: > On Tue, Jul 4, 2017 at 8:36 PM, Krzysztof Kozlowski wrote: > > On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote: > >> Hi Krzysztof, > >> > >> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski > >> wrote: > >> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote: > >> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski > >> >> wrote: > >> >> > genpd_syscore_switch() had two problems: > >> >> > 1. It silently assumed that device, it is being called for, belongs to > >> >> >generic power domain and used container_of() on its power domain > >> >> >pointer. Such assumption might not be true always. > >> >> > > >> >> > 2. It iterated over list of generic power domains without holding > >> >> >gpd_list_lock mutex thus list could have been modified in the same > >> >> >time. > >> >> > > >> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call > >> >> > for non-generic power domains and uses mutex when iterating. > >> >> > > >> >> > Reported-by: Ulf Hansson > >> >> > Signed-off-by: Krzysztof Kozlowski > >> >> > Acked-by: Ulf Hansson > >> >> > >> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull > >> >> request "[GIT PULL] Power management updates for v4.13-rc1". > >> >> > >> >> > Not tested on real hardware. > >> >> > >> >> This causes the following BUG during s2ram on all my Renesas arm32 > >> >> boards, > >> >> where the system timer is an IRQ safe device: > >> >> > >> >> PM: Syncing filesystems ... done. > >> >> PM: Preparing system for sleep (mem) > >> >> Freezing user space processes ... (elapsed 0.001 seconds) done. > >> >> OOM killer disabled. > >> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > >> >> PM: Suspending system (mem) > >> >> PM: suspend of devices complete after 122.841 msecs > >> >> PM: suspend devices took 0.130 seconds > >> >> PM: late suspend of devices complete after 2.682 msecs > >> >> PM: noirq suspend of devices complete after 29.951 msecs > >> >> Disabling non-boot CPUs ... > >> >> BUG: sleeping function called from invalid context at > >> >> kernel/locking/mutex.c:238 > >> > > >> > Thanks for report! > >> > > >> > Damn it, although I couldn't find this in the code, but I was fearing > >> > that this ends up in atomic section. That would kind of explain why > >> > mutex was not there [1]. > >> > > >> > Anyway, the buggy code was there already. Instead of "sleeping in atomic > >> > section" there was no locking at all... In this context this was > >> > probably safe because it was executed *after* disabling non-boot CPUs > >> > but then the function cannot be called in other contexts. > >> > > >> > I am not sure I will be capable of developing the proper fix as I do not > >> > have the hardware and I do not know all stuff happening in sh suspend. > >> > Probably reverting this and living with non-locked path would be the > >> > safest choice. > >> > > >> > [1] https://patchwork.kernel.org/patch/9778903/ > >> > >> AFAIU, all syscore stuff runs in atomic context. > > > > Indeed... The confusing part is that this code is syscore only from > > the name, it is not hooked in to syscore_ops. Although going by call > > chain (through sh clocksource drivers) we end up in > > timekeeping_suspend() which is a syscore op. > > > > I wonder whether it would be useful - after reverting my commit - to add > > an assert (which is a stronger API requirement than only documentation "may > > only be called during the system core (syscore) suspend") like: > > WARN_ON(num_online_cpus() > 1)); > > as without mutexes this should not be executed with more than one online > > CPU. > > Or maybe WARN_ON_ONCE(!in_atomic())? You could be in atomic section on this CPU and still have other CPUs online playing with gpd_list (without any protection from locking). This function is safe only on non-SMP case. Best regards, Krzysztof > I'm queuing up a revert of the $subject commit. > > Thanks, > Rafael
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski wrote: > On Tue, Jul 04, 2017 at 09:54:10PM +0200, Rafael J. Wysocki wrote: >> On Tue, Jul 4, 2017 at 8:36 PM, Krzysztof Kozlowski wrote: >> > On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote: >> >> Hi Krzysztof, >> >> >> >> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski >> >> wrote: >> >> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote: >> >> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski >> >> >> wrote: >> >> >> > genpd_syscore_switch() had two problems: >> >> >> > 1. It silently assumed that device, it is being called for, belongs >> >> >> > to >> >> >> >generic power domain and used container_of() on its power domain >> >> >> >pointer. Such assumption might not be true always. >> >> >> > >> >> >> > 2. It iterated over list of generic power domains without holding >> >> >> >gpd_list_lock mutex thus list could have been modified in the same >> >> >> >time. >> >> >> > >> >> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call >> >> >> > for non-generic power domains and uses mutex when iterating. >> >> >> > >> >> >> > Reported-by: Ulf Hansson >> >> >> > Signed-off-by: Krzysztof Kozlowski >> >> >> > Acked-by: Ulf Hansson >> >> >> >> >> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull >> >> >> request "[GIT PULL] Power management updates for v4.13-rc1". >> >> >> >> >> >> > Not tested on real hardware. >> >> >> >> >> >> This causes the following BUG during s2ram on all my Renesas arm32 >> >> >> boards, >> >> >> where the system timer is an IRQ safe device: >> >> >> >> >> >> PM: Syncing filesystems ... done. >> >> >> PM: Preparing system for sleep (mem) >> >> >> Freezing user space processes ... (elapsed 0.001 seconds) done. >> >> >> OOM killer disabled. >> >> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. >> >> >> PM: Suspending system (mem) >> >> >> PM: suspend of devices complete after 122.841 msecs >> >> >> PM: suspend devices took 0.130 seconds >> >> >> PM: late suspend of devices complete after 2.682 msecs >> >> >> PM: noirq suspend of devices complete after 29.951 msecs >> >> >> Disabling non-boot CPUs ... >> >> >> BUG: sleeping function called from invalid context at >> >> >> kernel/locking/mutex.c:238 >> >> > >> >> > Thanks for report! >> >> > >> >> > Damn it, although I couldn't find this in the code, but I was fearing >> >> > that this ends up in atomic section. That would kind of explain why >> >> > mutex was not there [1]. >> >> > >> >> > Anyway, the buggy code was there already. Instead of "sleeping in atomic >> >> > section" there was no locking at all... In this context this was >> >> > probably safe because it was executed *after* disabling non-boot CPUs >> >> > but then the function cannot be called in other contexts. >> >> > >> >> > I am not sure I will be capable of developing the proper fix as I do not >> >> > have the hardware and I do not know all stuff happening in sh suspend. >> >> > Probably reverting this and living with non-locked path would be the >> >> > safest choice. >> >> > >> >> > [1] https://patchwork.kernel.org/patch/9778903/ >> >> >> >> AFAIU, all syscore stuff runs in atomic context. >> > >> > Indeed... The confusing part is that this code is syscore only from >> > the name, it is not hooked in to syscore_ops. Although going by call >> > chain (through sh clocksource drivers) we end up in >> > timekeeping_suspend() which is a syscore op. >> > >> > I wonder whether it would be useful - after reverting my commit - to add >> > an assert (which is a stronger API requirement than only documentation "may >> > only be called during the system core (syscore) suspend") like: >> > WARN_ON(num_online_cpus() > 1)); >> > as without mutexes this should not be executed with more than one online >> > CPU. >> >> Or maybe WARN_ON_ONCE(!in_atomic())? > > You could be in atomic section on this CPU and still have other CPUs > online playing with gpd_list (without any protection from locking). > This function is safe only on non-SMP case. Well, not quite. It is safe if you can guarantee that no other CPUs will touch the data structure in question concurrently, which pretty much is the case for timekeeping_suspend() even though it may be invoked without taking the other CPUs offline (from the suspend-to-idle core path). Thanks, Rafael
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 04, 2017 at 10:12:13PM +0200, Rafael J. Wysocki wrote: > On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski wrote: >> >> > Thanks for report! > >> >> > > >> >> > Damn it, although I couldn't find this in the code, but I was fearing > >> >> > that this ends up in atomic section. That would kind of explain why > >> >> > mutex was not there [1]. > >> >> > > >> >> > Anyway, the buggy code was there already. Instead of "sleeping in > >> >> > atomic > >> >> > section" there was no locking at all... In this context this was > >> >> > probably safe because it was executed *after* disabling non-boot CPUs > >> >> > but then the function cannot be called in other contexts. > >> >> > > >> >> > I am not sure I will be capable of developing the proper fix as I do > >> >> > not > >> >> > have the hardware and I do not know all stuff happening in sh suspend. > >> >> > Probably reverting this and living with non-locked path would be the > >> >> > safest choice. > >> >> > > >> >> > [1] https://patchwork.kernel.org/patch/9778903/ > >> >> > >> >> AFAIU, all syscore stuff runs in atomic context. > >> > > >> > Indeed... The confusing part is that this code is syscore only from > >> > the name, it is not hooked in to syscore_ops. Although going by call > >> > chain (through sh clocksource drivers) we end up in > >> > timekeeping_suspend() which is a syscore op. > >> > > >> > I wonder whether it would be useful - after reverting my commit - to add > >> > an assert (which is a stronger API requirement than only documentation > >> > "may > >> > only be called during the system core (syscore) suspend") like: > >> > WARN_ON(num_online_cpus() > 1)); > >> > as without mutexes this should not be executed with more than one online > >> > CPU. > >> > >> Or maybe WARN_ON_ONCE(!in_atomic())? > > > > You could be in atomic section on this CPU and still have other CPUs > > online playing with gpd_list (without any protection from locking). > > This function is safe only on non-SMP case. > > Well, not quite. > > It is safe if you can guarantee that no other CPUs will touch the data > structure in question concurrently, which pretty much is the case for > timekeeping_suspend() even though it may be invoked without taking the > other CPUs offline (from the suspend-to-idle core path). Right, that would work fine for that case. However I was rather thinking that we have an in-kernel API (exported) so someone might by mistake try to use it in different contexts. For example in some atomic section but on a platform which offlines CPUs later. Thus it would be called in some imaginary suspend path but with CPUs still being online. Partially it is already mentioned in documentation although I am not sure that on every possible architecture syscore ops are called after disabling non-boot CPUs... And anyway for in-kernel API having a explicit assert is a stronger way of documenting requirement than a comment. Best regards, Krzysztof
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 4, 2017 at 10:20 PM, Krzysztof Kozlowski wrote: > On Tue, Jul 04, 2017 at 10:12:13PM +0200, Rafael J. Wysocki wrote: >> On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski wrote: > >> >> > Thanks for report! >> >> >> > >> >> >> > Damn it, although I couldn't find this in the code, but I was fearing >> >> >> > that this ends up in atomic section. That would kind of explain why >> >> >> > mutex was not there [1]. >> >> >> > >> >> >> > Anyway, the buggy code was there already. Instead of "sleeping in >> >> >> > atomic >> >> >> > section" there was no locking at all... In this context this was >> >> >> > probably safe because it was executed *after* disabling non-boot CPUs >> >> >> > but then the function cannot be called in other contexts. >> >> >> > >> >> >> > I am not sure I will be capable of developing the proper fix as I do >> >> >> > not >> >> >> > have the hardware and I do not know all stuff happening in sh >> >> >> > suspend. >> >> >> > Probably reverting this and living with non-locked path would be the >> >> >> > safest choice. >> >> >> > >> >> >> > [1] https://patchwork.kernel.org/patch/9778903/ >> >> >> >> >> >> AFAIU, all syscore stuff runs in atomic context. >> >> > >> >> > Indeed... The confusing part is that this code is syscore only from >> >> > the name, it is not hooked in to syscore_ops. Although going by call >> >> > chain (through sh clocksource drivers) we end up in >> >> > timekeeping_suspend() which is a syscore op. >> >> > >> >> > I wonder whether it would be useful - after reverting my commit - to add >> >> > an assert (which is a stronger API requirement than only documentation >> >> > "may >> >> > only be called during the system core (syscore) suspend") like: >> >> > WARN_ON(num_online_cpus() > 1)); >> >> > as without mutexes this should not be executed with more than one online >> >> > CPU. >> >> >> >> Or maybe WARN_ON_ONCE(!in_atomic())? >> > >> > You could be in atomic section on this CPU and still have other CPUs >> > online playing with gpd_list (without any protection from locking). >> > This function is safe only on non-SMP case. >> >> Well, not quite. >> >> It is safe if you can guarantee that no other CPUs will touch the data >> structure in question concurrently, which pretty much is the case for >> timekeeping_suspend() even though it may be invoked without taking the >> other CPUs offline (from the suspend-to-idle core path). > > Right, that would work fine for that case. > > However I was rather thinking that we have an in-kernel API (exported) > so someone might by mistake try to use it in different contexts. For > example in some atomic section but on a platform which offlines CPUs > later. Thus it would be called in some imaginary suspend path but with > CPUs still being online. Partially it is already mentioned in documentation > although I am not sure that on every possible architecture syscore ops > are called after disabling non-boot CPUs... Yes, they are. Nonboot CPUs are disabled by the core. Anyway, while I see your point, it would be rather hard to find an assertion that would also work for the suspend-to-idle timekeeping_suspend() invocation case. Thanks, Rafael
