devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Exynos 5410 Dual cluster support
@ 2013-10-14 15:08 Vyacheslav Tyrtov
  2013-10-14 15:08 ` [PATCH v2 1/4] ARM: EXYNOS: Add support for EXYNOS5410 SoC Vyacheslav Tyrtov
                   ` (4 more replies)
  0 siblings, 5 replies; 31+ messages in thread
From: Vyacheslav Tyrtov @ 2013-10-14 15:08 UTC (permalink / raw)
  To: linux-kernel
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren,
	Ian Campbell, Rob Landley, Kukjin Kim, Russell King, Ben Dooks,
	Mike Turquette, Daniel Lezcano, Thomas Gleixner, Heiko Stuebner,
	Naour Romain, devicetree, linux-doc, linux-arm-kernel,
	linux-samsung-soc, Tarek Dakhran, Tyrtov Vyacheslav

The series of patches represent support of Exynos 5410 SoC
    
The Exynos 5410 is the first Samsung SoC based on bigLITTLE architecture.
Patches allow all 8 CPU cores (4 x A7 and 4 x A15) to run at the same time
    
Patches add new platform description, support of clock controller,
dual cluster support and device tree for Exynos 5410 
   
Has been build on v3.12-rc5.
Has been tested on Exynos 5410 reference board (exynos_defconfig).
    
Thanks,
	Vyacheslav.


Changelog:

v2:
	In drivers/clk/samsung/clk-exynos5410.c
1. Clock driver converted to use preprocessor macros instead of enums.
   Clock IDs now defined in include/dt-bindings/clock/exynos5410.h.
2. Unused spinlock removed.
3. Function exynos5410_clk_init defined as static.
   Struct exynos5410_fixed_rate_ext_clks defined as static.
   Struct exynos5410_mux_clks defined as static.
   Struct exynos5410_div_clks defined as static.
   Struct exynos5410_gate_clks defined as static.
4. Removed aliases.
5. Pll's magic register offsets defined as preprocessor macros.
6. Redundant check of device_node pointer removed.
	
	In arch/arm/boot/dts/exynos5410.dtsi
1. dwmmcX nodes renamed to mmc.
   dwmmc_X renamed to mmc_X.
   dwmmc status="disabled" field added.
   fifo-depth field moved from arch/arm/boot/dts/exynos5410-smdk5410.dts
2. Blank lines added where necessary.
3. cpu@ suffixes corrected.
4. edcs node removed.
5. Hexadecimal characters case corrected.
6. Clock IDs replaced with preprocessor macros.
	
	In arch/arm/boot/dts/exynos5410-smdk5410.dts
1. status = "okay" field added to mmc nodes.
	
	In arch/arm/mach-exynos/edcs.c
1. "kfs_" prefix replaced with "edcs_"
2. EDCS_CPUS_PER_CLUSTER and EDCS_CLUSTERS defined instead of MCPM's values.
3. Cache handling sequence borrowed from arch/arm/mach-vexpress/tc2_pm.c
4. mcpm_sync_init() call added.
5. power management functions reworked.
	
	Other
1. Documentation/devicetree/bindings/clock/exynos5410-clock.txt corrected.
2. Removed smdk5410_defconfig. Instead SOC_EXYNOS5410 now selects MCPM and
   ARM_CCI in arch/arm/mach-exynos/Kconfig.
3. edcs_status driver removed.

Tarek Dakhran (4):
  ARM: EXYNOS: Add support for EXYNOS5410 SoC
  clk: exynos5410: register clocks using common clock framework
  ARM: EXYNOS: add Exynos Dual Cluster Support
  ARM: dts: Add initial device tree support for EXYNOS5410

 .../devicetree/bindings/clock/exynos5410-clock.txt |  37 +++
 arch/arm/boot/dts/Makefile                         |   1 +
 arch/arm/boot/dts/exynos5410-smdk5410.dts          |  65 +++++
 arch/arm/boot/dts/exynos5410.dtsi                  | 209 ++++++++++++++++
 arch/arm/mach-exynos/Kconfig                       |  12 +
 arch/arm/mach-exynos/Makefile                      |   2 +
 arch/arm/mach-exynos/common.c                      |  18 ++
 arch/arm/mach-exynos/edcs.c                        | 270 +++++++++++++++++++++
 arch/arm/mach-exynos/include/mach/map.h            |   1 +
 arch/arm/mach-exynos/mach-exynos5-dt.c             |   1 +
 arch/arm/plat-samsung/include/plat/cpu.h           |   8 +
 drivers/clk/samsung/Makefile                       |   1 +
 drivers/clk/samsung/clk-exynos5410.c               | 251 +++++++++++++++++++
 include/dt-bindings/clock/exynos5410.h             | 175 +++++++++++++
 14 files changed, 1051 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/exynos5410-clock.txt
 create mode 100644 arch/arm/boot/dts/exynos5410-smdk5410.dts
 create mode 100644 arch/arm/boot/dts/exynos5410.dtsi
 create mode 100644 arch/arm/mach-exynos/edcs.c
 create mode 100644 drivers/clk/samsung/clk-exynos5410.c
 create mode 100644 include/dt-bindings/clock/exynos5410.h

-- 
1.8.1.5

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
@ 2013-10-17 14:32 Dave Martin
  2013-10-17 16:04 ` Daniel Lezcano
  0 siblings, 1 reply; 31+ messages in thread
From: Dave Martin @ 2013-10-17 14:32 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: Vyacheslav Tyrtov, linux-kernel, Mark Rutland, devicetree,
	Kukjin Kim, Russell King, Ben Dooks, Pawel Moll, Ian Campbell,
	Nicolas Pitre, Stephen Warren, linux-doc, Rob Herring,
	Tarek Dakhran, linux-samsung-soc, Rob Landley, Mike Turquette,
	Thomas Gleixner, Naour Romain, Lorenzo Pieralisi,
	linux-arm-kernel, Heiko Stuebner

On Thu, Oct 17, 2013 at 12:45:29PM +0200, Daniel Lezcano wrote:
> On 10/14/2013 05:08 PM, Vyacheslav Tyrtov wrote:
> > From: Tarek Dakhran <t.dakhran@samsung.com>
> > 
> > Add EDCS(Exynos Dual Cluster Support) for Samsung Exynos5410 SoC.
> > This enables all 8 cores, 4 x A7 and 4 x A15 run at the same time.

[...]

> > +	__mcpm_cpu_down(cpu, cluster);
> > +
> > +	if (!skip_wfi) {
> > +		exynos_core_power_down(cpu, cluster);
> > +		wfi();
> > +	}
> > +}
> 
> I did not looked line by line but these functions looks very similar
> than the tc2_pm.c's function. no ?

This is true.

> May be some code consolidation could be considered here.
> 
> Added Nico and Lorenzo in Cc.
> 
> Thanks
>   -- Daniel

Nico can commnent further, but I think the main concern here was that
this code shouldn't be factored prematurely.

There are many low-level platform specifics involved here, so it's
hard to be certain that all platforms could fit into a more abstracted
framework until we have some evidence to look at.

This could be revisited when we have a few diverse MCPM ports to
compare.


The low-level A15/A7 cacheflush sequence is already being factored
by Nico [1].

Cheers
---Dave

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/205085.html
[PATCH] ARM: cacheflush: consolidate single-CPU ARMv7 cache disabling code

[...]

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
@ 2013-10-17 15:46 Dave Martin
  2013-10-18  8:29 ` Tarek Dakhran
  0 siblings, 1 reply; 31+ messages in thread
From: Dave Martin @ 2013-10-17 15:46 UTC (permalink / raw)
  To: Vyacheslav Tyrtov
  Cc: linux-kernel, Mark Rutland, devicetree, Kukjin Kim, Russell King,
	Ben Dooks, Pawel Moll, Ian Campbell, Stephen Warren, linux-doc,
	Rob Herring, Tarek Dakhran, Daniel Lezcano, linux-samsung-soc,
	Rob Landley, Mike Turquette, Thomas Gleixner, Naour Romain,
	linux-arm-kernel, Heiko Stuebner

On Mon, Oct 14, 2013 at 07:08:24PM +0400, Vyacheslav Tyrtov wrote:
> From: Tarek Dakhran <t.dakhran@samsung.com>
> 
> Add EDCS(Exynos Dual Cluster Support) for Samsung Exynos5410 SoC.
> This enables all 8 cores, 4 x A7 and 4 x A15 run at the same time.
> 
> Signed-off-by: Tarek Dakhran <t.dakhran@samsung.com>
> Signed-off-by: Vyacheslav Tyrtov <v.tyrtov@samsung.com>
> ---
>  arch/arm/mach-exynos/Makefile |   2 +
>  arch/arm/mach-exynos/edcs.c   | 270 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 272 insertions(+)
>  create mode 100644 arch/arm/mach-exynos/edcs.c
> 
> diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
> index 5369615..ba6efdb 100644
> --- a/arch/arm/mach-exynos/Makefile
> +++ b/arch/arm/mach-exynos/Makefile
> @@ -34,3 +34,5 @@ AFLAGS_exynos-smc.o		:=-Wa,-march=armv7-a$(plus_sec)
>  
>  obj-$(CONFIG_MACH_EXYNOS4_DT)		+= mach-exynos4-dt.o
>  obj-$(CONFIG_MACH_EXYNOS5_DT)		+= mach-exynos5-dt.o
> +
> +obj-$(CONFIG_SOC_EXYNOS5410)		+= edcs.o
> diff --git a/arch/arm/mach-exynos/edcs.c b/arch/arm/mach-exynos/edcs.c
> new file mode 100644
> index 0000000..e304bd9
> --- /dev/null
> +++ b/arch/arm/mach-exynos/edcs.c
> @@ -0,0 +1,270 @@
> +/*
> + * arch/arm/mach-exynos/edcs.c - exynos dual cluster power management support
> + *
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * Author: Tarek Dakhran <t.dakhran@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * EDCS(exynos dual cluster support) for Exynos5410 SoC.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/spinlock.h>
> +#include <linux/errno.h>
> +
> +#include <asm/mcpm.h>
> +#include <asm/proc-fns.h>
> +#include <asm/cacheflush.h>
> +#include <asm/cputype.h>
> +#include <asm/cp15.h>
> +
> +#include <linux/arm-cci.h>
> +#include <mach/regs-pmu.h>
> +
> +#define EDCS_CPUS_PER_CLUSTER	4
> +#define EDCS_CLUSTERS		2
> +
> +/* Exynos5410 power management registers */
> +#define EDCS_CORE_CONFIGURATION(_nr)	(S5P_ARM_CORE0_CONFIGURATION	\
> +						+ ((_nr) * 0x80))
> +#define EDCS_CORE_STATUS(_nr)		(EDCS_CORE_CONFIGURATION(_nr) + 0x4)
> +#define EDCS_CORE_OPTION(_nr)		(EDCS_CORE_CONFIGURATION(_nr) + 0x8)
> +
> +#define REG_CPU_STATE_ADDR0		(S5P_VA_SYSRAM_NS + 0x28)

Is there any reason why S5P_VA_SYSRAM_NS needs to be a static mapping?

> +#define REG_CPU_STATE_ADDR(_nr)		(REG_CPU_STATE_ADDR0 +	\
> +						 _nr * EDCS_CPUS_PER_CLUSTER)
> +
> +static arch_spinlock_t edcs_lock = __ARCH_SPIN_LOCK_UNLOCKED;
> +
> +static int edcs_use_count[EDCS_CPUS_PER_CLUSTER][EDCS_CLUSTERS];
> +static int core_count[EDCS_CLUSTERS];
> +
> +static void exynos_core_power_control(unsigned int cpu, unsigned int cluster,
> +				bool enable)
> +{
> +	unsigned int offset = cluster * EDCS_CPUS_PER_CLUSTER + cpu;
> +	int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
> +
> +	if ((__raw_readl(EDCS_CORE_STATUS(offset)) & 0x3) != value)
> +		__raw_writel(value, EDCS_CORE_CONFIGURATION(offset));

I think you need to replace all the __raw_readl() / __raw_writel() calls
in this file with readl_relaxed() / writel_relaxed().

This ensures little-endian byte order, so that BE8 kernels will work.

> +}
> +
> +static void exynos_core_power_up(unsigned int cpu, unsigned int cluster)
> +{
> +	exynos_core_power_control(cpu, cluster, true);
> +}
> +
> +static void exynos_core_power_down(unsigned int cpu, unsigned int cluster)
> +{
> +	exynos_core_power_control(cpu, cluster, false);
> +}
> +
> +void set_boot_flag(unsigned int cpu, unsigned int mode)
> +{
> +	__raw_writel(mode, REG_CPU_STATE_ADDR(cpu));
> +}
> +
> +static int exynos_power_up(unsigned int cpu, unsigned int cluster)
> +{
> +	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> +	BUG_ON(cpu >= EDCS_CPUS_PER_CLUSTER || cluster >= EDCS_CLUSTERS);
> +
> +	local_irq_disable();
> +	arch_spin_lock(&edcs_lock);
> +
> +	edcs_use_count[cpu][cluster]++;
> +	if (edcs_use_count[cpu][cluster] == 1) {
> +		++core_count[cluster];
> +		set_boot_flag(cpu, 0x2);

0x2 looks like a magic number.  Can we have a #define for that?


If the boot flag is read by the inbound CPU or by a peripheral then
there are memory ordering issues to take into account.  Otherwise, can't
the inbound CPU come online and race with the write to the boot flag?

What is the memory type of REG_STATE_ADDR()?

> +		exynos_core_power_up(cpu, cluster);
> +	} else if (edcs_use_count[cpu][cluster] != 2) {
> +		/*
> +		 * The only possible values are:
> +		 * 0 = CPU down
> +		 * 1 = CPU (still) up
> +		 * 2 = CPU requested to be up before it had a chance
> +		 *     to actually make itself down.
> +		 * Any other value is a bug.
> +		 */
> +		BUG();
> +	}
> +
> +	arch_spin_unlock(&edcs_lock);
> +	local_irq_enable();
> +
> +	return 0;
> +}
> +static void exynos_power_down(void)
> +{
> +	unsigned int mpidr, cpu, cluster;
> +	bool last_man = false, skip_wfi = false;
> +
> +	mpidr = read_cpuid_mpidr();
> +	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +
> +	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> +	BUG_ON(cpu >= EDCS_CPUS_PER_CLUSTER  || cluster >= EDCS_CLUSTERS);
> +
> +	__mcpm_cpu_going_down(cpu, cluster);
> +
> +	arch_spin_lock(&edcs_lock);
> +	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
> +	edcs_use_count[cpu][cluster]--;
> +	if (edcs_use_count[cpu][cluster] == 0) {
> +		--core_count[cluster];
> +		if (core_count[cluster] == 0)
> +			last_man = true;
> +	} else if (edcs_use_count[cpu][cluster] == 1) {
> +		/*
> +		 * A power_up request went ahead of us.
> +		 * Even if we do not want to shut this CPU down,
> +		 * the caller expects a certain state as if the WFI
> +		 * was aborted.  So let's continue with cache cleaning.
> +		 */
> +		skip_wfi = true;
> +	} else
> +		BUG();

For TC2, Lorenzo discovered need to disable the GIC CPU interface, so
that no suprious wakeups can happen once the power controller is
configured to power the CPU down.

The problem is that a spurious wakeup can trigger a race in the power
controller when the last man powers down, where the power controller
continues to wait for the cluster to power down, but it never happens.

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/200917.html
([PATCH v2] arm: vexpress: tc2: fix hotplug/idle/kexec race on cluster power down)


This might not be a problem for Exynos5410 -- it depends on the way the
power control logic works.

On the other hand, disabling the GIC CPU interface here is cheap to do,
even if it's not really needed.

> +
> +	if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
> +		arch_spin_unlock(&edcs_lock);
> +
> +		if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A15) {
> +			/*
> +			 * On the Cortex-A15 we need to disable
> +			 * L2 prefetching before flushing the cache.
> +			 */
> +			asm volatile(
> +			"mcr	p15, 1, %0, c15, c0, 3\n\t"
> +			"isb\n\t"
> +			"dsb"
> +			: : "r" (0x400));
> +		}
> +
> +		/*
> +		 * We need to disable and flush the whole (L1 and L2) cache.
> +		 * Let's do it in the safest possible way i.e. with
> +		 * no memory access within the following sequence
> +		 * including the stack.
> +		 *
> +		 * Note: fp is preserved to the stack explicitly prior doing
> +		 * this since adding it to the clobber list is incompatible
> +		 * with having CONFIG_FRAME_POINTER=y.
> +		 */
> +		asm volatile(
> +		"str	fp, [sp, #-4]!\n\t"
> +		"mrc	p15, 0, r0, c1, c0, 0	@ get CR\n\t"
> +		"bic	r0, r0, #"__stringify(CR_C)"\n\t"
> +		"mcr	p15, 0, r0, c1, c0, 0	@ set CR\n\t"
> +		"isb\n\t"
> +		"bl	v7_flush_dcache_all\n\t"
> +		"clrex\n\t"
> +		"mrc	p15, 0, r0, c1, c0, 1	@ get AUXCR\n\t"
> +		"bic	r0, r0, #(1 << 6)	@ disable local coherency\n\t"
> +		"mcr	p15, 0, r0, c1, c0, 1	@ set AUXCR\n\t"
> +		"isb\n\t"
> +		"dsb\n\t"
> +		"ldr	fp, [sp], #4"
> +		: : : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
> +			"r9", "r10", "lr", "memory");

For these code sequences, please take a look at

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/205085.html
[PATCH] ARM: cacheflush: consolidate single-CPU ARMv7 cache disabling code

The aim is to consolidate on a macro that can be shared between
backends.

> +
> +		cci_disable_port_by_cpu(mpidr);
> +
> +		__mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
> +
> +	} else {
> +		arch_spin_unlock(&edcs_lock);
> +		/*
> +			* We need to disable and flush only the L1 cache.
> +			* Let's do it in the safest possible way as above.
> +		*/
> +		asm volatile(
> +		"str	fp, [sp, #-4]!\n\t"
> +		"mrc	p15, 0, r0, c1, c0, 0	@ get CR\n\t"
> +		"bic	r0, r0, #"__stringify(CR_C)"\n\t"
> +		"mcr	p15, 0, r0, c1, c0, 0	@ set CR\n\t"
> +		"isb\n\t"
> +		"bl	v7_flush_dcache_louis\n\t"
> +		"clrex\n\t"
> +		"mrc	p15, 0, r0, c1, c0, 1	@ get AUXCR\n\t"
> +		"bic	r0, r0, #(1 << 6)	@ disable local coherency\n\t"
> +		"mcr	p15, 0, r0, c1, c0, 1	@ set AUXCR\n\t"
> +		"isb\n\t"
> +		"dsb\n\t"
> +		"ldr	fp, [sp], #4"
> +		: : : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
> +		      "r9", "r10", "lr", "memory");
> +
> +	}
> +	__mcpm_cpu_down(cpu, cluster);
> +
> +	if (!skip_wfi) {
> +		exynos_core_power_down(cpu, cluster);
> +		wfi();
> +	}
> +}
> +
> +static const struct mcpm_platform_ops exynos_power_ops = {
> +	.power_up	= exynos_power_up,
> +	.power_down	= exynos_power_down,
> +};
> +
> +static void __init edcs_data_init(void)
> +{
> +	unsigned int mpidr, cpu, cluster;
> +
> +	mpidr = read_cpuid_mpidr();
> +	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +
> +	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> +	BUG_ON(cpu >= EDCS_CPUS_PER_CLUSTER  || cluster >= EDCS_CLUSTERS);
> +	edcs_use_count[cpu][cluster] = 1;
> +	++core_count[cluster];
> +}
> +
> +/*
> + * Enable cluster-level coherency, in preparation for turning on the MMU.
> + */
> +static void __naked edcs_power_up_setup(unsigned int affinity_level)
> +{
> +	asm volatile ("\n"
> +	"b	cci_enable_port_for_self");
> +}
> +
> +static int __init edcs_init(void)
> +{
> +	int ret;
> +	struct device_node *node;
> +
> +	node = of_find_compatible_node(NULL, NULL, "samsung,exynos5410");
> +	if (!node)
> +		return -ENODEV;
> +
> +	if (!cci_probed())
> +		return -ENODEV;
> +
> +	/*
> +	 * Future entries into the kernel can now go
> +	 * through the cluster entry vectors.
> +	 */
> +	__raw_writel(virt_to_phys(mcpm_entry_point),
> +				S5P_VA_SYSRAM_NS + 0x1c);

It would me more readable to have a #define to describe what
S5P_VA_SYSRAM_NS + 0x1c is.


Also, what is the memory type of S5P_VA_SYSRAM_NS?  How is the entry
point address value read?  Does the inbound CPU's boot ROM or firmware
read it?

If it is read via some non-coherent side-channel, or if S5P_VA_SYSRAM_NS
is mapped as normal memory then we would need some explicit
synchronisation, but I suspect this is not the case (?)

Cheers
---Dave

> +
> +	edcs_data_init();
> +	mcpm_smp_set_ops();
> +
> +	ret = mcpm_platform_register(&exynos_power_ops);
> +	if (!ret) {
> +		mcpm_sync_init(edcs_power_up_setup);
> +		pr_info("EDCS power management initialized\n");
> +	}
> +	return ret;
> +}
> +
> +early_initcall(edcs_init);
> -- 
> 1.8.1.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
@ 2013-10-17 16:49 Dave Martin
  0 siblings, 0 replies; 31+ messages in thread
From: Dave Martin @ 2013-10-17 16:49 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: Mark Rutland, Nicolas Pitre, Heiko Stuebner, linux-doc,
	Naour Romain, Tarek Dakhran, Kukjin Kim, Russell King,
	Stephen Warren, linux-samsung-soc, devicetree, Pawel Moll,
	Ian Campbell, Rob Herring, Lorenzo Pieralisi, Vyacheslav Tyrtov,
	Ben Dooks, Mike Turquette, Thomas Gleixner, linux-arm-kernel,
	linux-kernel, Rob Landley

On Thu, Oct 17, 2013 at 06:04:13PM +0200, Daniel Lezcano wrote:
> On 10/17/2013 04:32 PM, Dave Martin wrote:
> > On Thu, Oct 17, 2013 at 12:45:29PM +0200, Daniel Lezcano wrote:
> >> On 10/14/2013 05:08 PM, Vyacheslav Tyrtov wrote:
> >>> From: Tarek Dakhran <t.dakhran@samsung.com>
> >>> 
> >>> Add EDCS(Exynos Dual Cluster Support) for Samsung Exynos5410 SoC.
> >>> This enables all 8 cores, 4 x A7 and 4 x A15 run at the same time.
> > 
> > [...]
> > 
> >>> +	__mcpm_cpu_down(cpu, cluster);
> >>> +
> >>> +	if (!skip_wfi) {
> >>> +		exynos_core_power_down(cpu, cluster);
> >>> +		wfi();
> >>> +	}
> >>> +}
> >> 
> >> I did not looked line by line but these functions looks very similar
> >> than the tc2_pm.c's function. no ?
> > 
> > This is true.
> > 
> >> May be some code consolidation could be considered here.
> >> 
> >> Added Nico and Lorenzo in Cc.
> >> 
> >> Thanks
> >>    -- Daniel
> > 
> > Nico can commnent further, but I think the main concern here was that
> > this code shouldn't be factored prematurely.
> 
> I do not share this opinion.
> 
> > There are many low-level platform specifics involved here, so it's
> > hard to be certain that all platforms could fit into a more abstracted
> > framework until we have some evidence to look at.
> > 
> > This could be revisited when we have a few diverse MCPM ports to
> > compare.
> 
> I am worried about seeing more and more duplicated code around the ARM
> arch (eg. arm[64]/kernel/smp.c arm64/kernel/smp.c).
> 
> The cpuidle drivers have been duplicated and it took a while before
> refactoring them, and it is not finished. The hotplug code have been
> duplicated and now it is very difficult to factor it out.
> 
> A lot of work have been done to consolidate the code across the
> different SoC since the last 2 years.
> 
> If we let duplicate code populate the different files, they will
> belong to different maintainers, thus different trees. Each SoC
> contributors will tend to add their small changes making the code to
> diverge more and more and making difficult to re-factor it later.

I think this is Nico's call, since he has more experience than I do
of how these things tend to play out in practice.

Cheers
---Dave

> I am in favor of preventing duplicate code entering in the kernel and
> force the contributors to improve the kernel in general, not just the
> small part they are supposed to work on. Otherwise, we are letting the
> kernel to fork itself, internally...
> 
> > The low-level A15/A7 cacheflush sequence is already being factored
> > by Nico [1].
> 
> Hopefully he did it :)
> 
> Thanks
>   -- Daniel
> 
> > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/205085.html
> > [PATCH] ARM: cacheflush: consolidate single-CPU ARMv7 cache disabling code
> > 
> > [...]
> > 
> 
> 
> -- 
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
@ 2013-10-18 12:53 Dave Martin
  0 siblings, 0 replies; 31+ messages in thread
From: Dave Martin @ 2013-10-18 12:53 UTC (permalink / raw)
  To: Tarek Dakhran
  Cc: Vyacheslav Tyrtov, Mark Rutland, devicetree, Kukjin Kim,
	Russell King, Daniel Lezcano, Pawel Moll, Ian Campbell,
	Stephen Warren, linux-doc, linux-kernel, Rob Herring,
	linux-samsung-soc, Rob Landley, Ben Dooks, Mike Turquette,
	Thomas Gleixner, Naour Romain, linux-arm-kernel, Heiko Stuebner

On Fri, Oct 18, 2013 at 12:29:03PM +0400, Tarek Dakhran wrote:
> On 17.10.2013 19:46, Dave Martin wrote:
> > On Mon, Oct 14, 2013 at 07:08:24PM +0400, Vyacheslav Tyrtov wrote:
> >> From: Tarek Dakhran <t.dakhran@samsung.com>
> >> 
> >> Add EDCS(Exynos Dual Cluster Support) for Samsung Exynos5410 SoC.
> >> This enables all 8 cores, 4 x A7 and 4 x A15 run at the same time.
> >> 
> >> Signed-off-by: Tarek Dakhran <t.dakhran@samsung.com>
> >> Signed-off-by: Vyacheslav Tyrtov <v.tyrtov@samsung.com>
> >> ---
> >>   arch/arm/mach-exynos/Makefile |   2 +
> >>   arch/arm/mach-exynos/edcs.c   | 270 ++++++++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 272 insertions(+)
> >>   create mode 100644 arch/arm/mach-exynos/edcs.c
> >> 
> >> diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
> >> index 5369615..ba6efdb 100644
> >> --- a/arch/arm/mach-exynos/Makefile
> >> +++ b/arch/arm/mach-exynos/Makefile
> >> @@ -34,3 +34,5 @@ AFLAGS_exynos-smc.o		:=-Wa,-march=armv7-a$(plus_sec)
> >>   obj-$(CONFIG_MACH_EXYNOS4_DT)		+= mach-exynos4-dt.o
> >>   obj-$(CONFIG_MACH_EXYNOS5_DT)		+= mach-exynos5-dt.o
> >> +
> >> +obj-$(CONFIG_SOC_EXYNOS5410)		+= edcs.o
> >> diff --git a/arch/arm/mach-exynos/edcs.c b/arch/arm/mach-exynos/edcs.c
> >> new file mode 100644
> >> index 0000000..e304bd9
> >> --- /dev/null
> >> +++ b/arch/arm/mach-exynos/edcs.c
> >> @@ -0,0 +1,270 @@
> >> +/*
> >> + * arch/arm/mach-exynos/edcs.c - exynos dual cluster power management support
> >> + *
> >> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> >> + * Author: Tarek Dakhran <t.dakhran@samsung.com>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * EDCS(exynos dual cluster support) for Exynos5410 SoC.
> >> + */
> >> +

> [snip]

> >> +#define EDCS_CORE_STATUS(_nr)		(EDCS_CORE_CONFIGURATION(_nr) + 0x4)
> >> +#define EDCS_CORE_OPTION(_nr)		(EDCS_CORE_CONFIGURATION(_nr) + 0x8)
> >> +
> >> +#define REG_CPU_STATE_ADDR0		(S5P_VA_SYSRAM_NS + 0x28)
> > Is there any reason why S5P_VA_SYSRAM_NS needs to be a static mapping?

> What do you mean by "static mapping"?

See below

> > 
> >> +#define REG_CPU_STATE_ADDR(_nr)		(REG_CPU_STATE_ADDR0 +	\
> >> +						 _nr * EDCS_CPUS_PER_CLUSTER)
> >> +
> >> +static arch_spinlock_t edcs_lock = __ARCH_SPIN_LOCK_UNLOCKED;
> >> +
> >> +static int edcs_use_count[EDCS_CPUS_PER_CLUSTER][EDCS_CLUSTERS];
> >> +static int core_count[EDCS_CLUSTERS];
> >> +
> >> +static void exynos_core_power_control(unsigned int cpu, unsigned int cluster,
> >> +				bool enable)
> >> +{
> >> +	unsigned int offset = cluster * EDCS_CPUS_PER_CLUSTER + cpu;
> >> +	int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
> >> +
> >> +	if ((__raw_readl(EDCS_CORE_STATUS(offset)) & 0x3) != value)
> >> +		__raw_writel(value, EDCS_CORE_CONFIGURATION(offset));
> > I think you need to replace all the __raw_readl() / __raw_writel() calls
> > in this file with readl_relaxed() / writel_relaxed().
> > 
> > This ensures little-endian byte order, so that BE8 kernels will work.
> > 
> Will be done.
> >> +}
> >> +
> >> +static void exynos_core_power_up(unsigned int cpu, unsigned int cluster)
> >> +{
> >> +	exynos_core_power_control(cpu, cluster, true);
> >> +}

> [snip]

> >> +
> >> +	edcs_use_count[cpu][cluster]++;
> >> +	if (edcs_use_count[cpu][cluster] == 1) {
> >> +		++core_count[cluster];
> >> +		set_boot_flag(cpu, 0x2);
> > 0x2 looks like a magic number.  Can we have a #define for that?

> Will be done.

Thanks

> > If the boot flag is read by the inbound CPU or by a peripheral then
> > there are memory ordering issues to take into account.  Otherwise, can't
> > the inbound CPU come online and race with the write to the boot flag?

> Inbound CPU doesn't write the boot flag.

Sorry, I didn't explain this clearly.  The scenario I'm thinking about is:

	CPU0 writes 2 to the boot flag

	The write is temporarily held in a write buffer somewhere in the
	memory system.

	CPU0 pokes the power controller to bring CPU1 online

	CPU1 powers up and starts running its boot ROM

	CPU1 reads the boot flag, but misses the value written by CPU0
	because that is still held in a write buffer somewhere, and isn't
	globally visible.

	... and finally ...

	The write to the boot flag drains and becomes globally visible.
	But it's too late now.

There's also the problem that set_boot_flag() and the read by the
boot ROM probably don't use the same memory type (Device versus
Strongly-Ordered for example).  The ARM Architecture doesn't guarantee
full coherency in situations like this.


I believe that a correct solution to this problem is to put a wmb()
between setting the boot flag and poking the power controller.

Putting the wmb() before the writel in exynos_core_power_control()
should do the job.

> > What is the memory type of REG_STATE_ADDR()?
> Same type as S5P_VA_SYSRAM_NS.
> This 4K region is mapped as follows:
> 
> static struct map_desc exynos5410_iodesc[] __initdata = {
> 	{
> 		.virtual	= (unsigned long)S5P_VA_SYSRAM_NS,
> 		.pfn		= __phys_to_pfn(EXYNOS5410_PA_SYSRAM_NS),
> 		.length		= SZ_4K,
> 		.type		= MT_DEVICE,
> 	},
> };

This is what I mean by a static mapping.

If this mapping isn't needed early (i.e., before ioremap works) then
ioremap can be used instead of reserving a specific virtual address.

This isn't a problem for this patch, just nice to have in general.

[...]

> >> +	arch_spin_lock(&edcs_lock);
> >> +	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
> >> +	edcs_use_count[cpu][cluster]--;
> >> +	if (edcs_use_count[cpu][cluster] == 0) {
> >> +		--core_count[cluster];
> >> +		if (core_count[cluster] == 0)
> >> +			last_man = true;
> >> +	} else if (edcs_use_count[cpu][cluster] == 1) {
> >> +		/*
> >> +		 * A power_up request went ahead of us.
> >> +		 * Even if we do not want to shut this CPU down,
> >> +		 * the caller expects a certain state as if the WFI
> >> +		 * was aborted.  So let's continue with cache cleaning.
> >> +		 */
> >> +		skip_wfi = true;
> >> +	} else
> >> +		BUG();
> > For TC2, Lorenzo discovered need to disable the GIC CPU interface, so
> > that no suprious wakeups can happen once the power controller is
> > configured to power the CPU down.
> > 
> > The problem is that a spurious wakeup can trigger a race in the power
> > controller when the last man powers down, where the power controller
> > continues to wait for the cluster to power down, but it never happens.
> > 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/200917.html
> > ([PATCH v2] arm: vexpress: tc2: fix hotplug/idle/kexec race on cluster power down)
> > 
> > 
> > This might not be a problem for Exynos5410 -- it depends on the way the
> > power control logic works.
> > 
> > On the other hand, disabling the GIC CPU interface here is cheap to do,
> > even if it's not really needed.

Did you have any further thoughts on this issue?

> >> +
> >> +	if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
> >> +		arch_spin_unlock(&edcs_lock);
> >> +
> >> +		if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A15) {
> >> +			/*
> >> +			 * On the Cortex-A15 we need to disable
> >> +			 * L2 prefetching before flushing the cache.
> >> +			 */
> >> +			asm volatile(
> >> +			"mcr	p15, 1, %0, c15, c0, 3\n\t"
> >> +			"isb\n\t"
> >> +			"dsb"
> >> +			: : "r" (0x400));
> >> +		}
> >> +
> >> +		/*
> >> +		 * We need to disable and flush the whole (L1 and L2) cache.
> >> +		 * Let's do it in the safest possible way i.e. with
> >> +		 * no memory access within the following sequence
> >> +		 * including the stack.
> >> +		 *
> >> +		 * Note: fp is preserved to the stack explicitly prior doing
> >> +		 * this since adding it to the clobber list is incompatible
> >> +		 * with having CONFIG_FRAME_POINTER=y.
> >> +		 */
> >> +		asm volatile(
> >> +		"str	fp, [sp, #-4]!\n\t"
> >> +		"mrc	p15, 0, r0, c1, c0, 0	@ get CR\n\t"
> >> +		"bic	r0, r0, #"__stringify(CR_C)"\n\t"
> >> +		"mcr	p15, 0, r0, c1, c0, 0	@ set CR\n\t"
> >> +		"isb\n\t"
> >> +		"bl	v7_flush_dcache_all\n\t"
> >> +		"clrex\n\t"
> >> +		"mrc	p15, 0, r0, c1, c0, 1	@ get AUXCR\n\t"
> >> +		"bic	r0, r0, #(1 << 6)	@ disable local coherency\n\t"
> >> +		"mcr	p15, 0, r0, c1, c0, 1	@ set AUXCR\n\t"
> >> +		"isb\n\t"
> >> +		"dsb\n\t"
> >> +		"ldr	fp, [sp], #4"
> >> +		: : : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
> >> +			"r9", "r10", "lr", "memory");
> > For these code sequences, please take a look at
> > 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/205085.html
> > [PATCH] ARM: cacheflush: consolidate single-CPU ARMv7 cache disabling code
> > 
> > The aim is to consolidate on a macro that can be shared between
> > backends.
> Will be done right after Nicolases patch will be applied to the mainline.

OK, keep an eye on it.

> >> +static int __init edcs_init(void)
> >> +{

[...]

> >> +	/*
> >> +	 * Future entries into the kernel can now go
> >> +	 * through the cluster entry vectors.
> >> +	 */
> >> +	__raw_writel(virt_to_phys(mcpm_entry_point),
> >> +				S5P_VA_SYSRAM_NS + 0x1c);
> > It would me more readable to have a #define to describe what
> > S5P_VA_SYSRAM_NS + 0x1c is.
> Will be done.

> > Also, what is the memory type of S5P_VA_SYSRAM_NS?  How is the entry
> > point address value read?  Does the inbound CPU's boot ROM or firmware
> > read it?

> The former (i.e. boot ROM reads it).

OK -- since that write is to device memory, I think we can assume in
practice that it will drain before the first secondary comes online.

If you follow the above advice about adding a wmb() in
exynos_core_power_control(), that should ensure correct ordering for
this too, if I've understood correctly.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 31+ messages in thread
end of thread, other threads:[~2013-11-05  9:15 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-14 15:08 [PATCH v2 0/4] Exynos 5410 Dual cluster support Vyacheslav Tyrtov
2013-10-14 15:08 ` [PATCH v2 1/4] ARM: EXYNOS: Add support for EXYNOS5410 SoC Vyacheslav Tyrtov
2013-11-01 17:08   ` Tomasz Figa
2013-11-01 21:52   ` Rob Herring
2013-11-01 23:02     ` Tomasz Figa
2013-10-14 15:08 ` [PATCH v2 2/4] clk: exynos5410: register clocks using common clock framework Vyacheslav Tyrtov
2013-10-22  9:44   ` Mike Turquette
2013-11-01 15:53     ` Mauro Ribeiro
2013-11-01 16:25       ` Mauro Ribeiro
2013-11-01 16:58   ` Tomasz Figa
2013-11-05  9:15     ` Tarek Dakhran
2013-10-14 15:08 ` [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support Vyacheslav Tyrtov
2013-10-17 10:45   ` Daniel Lezcano
2013-10-25 10:06   ` Aliaksei Katovich
2013-10-14 15:08 ` [PATCH v2 4/4] ARM: dts: Add initial device tree support for EXYNOS5410 Vyacheslav Tyrtov
     [not found]   ` <1381763305-7085-5-git-send-email-v.tyrtov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-11-01 17:09     ` Tomasz Figa
2013-10-16 22:15 ` [PATCH v2 0/4] Exynos 5410 Dual cluster support Kevin Hilman
     [not found]   ` <87k3hc3lso.fsf-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2013-10-17 12:24     ` Aliaksei Katovich
2013-10-17 13:04   ` Aliaksei Katovich
2013-10-17 13:51     ` Tarek Dakhran
2013-10-17 14:24       ` Aliaksei Katovich
2013-10-17 14:02     ` Tarek Dakhran
2013-10-17 20:00     ` Kevin Hilman
     [not found]       ` <CAGc3s_cbCgdPdbCPF2XN+p-O5RfM-4pNZT49=_4MOVy=Lz8d=Q@mail.gmail.com>
2013-10-30 23:57         ` Kevin Hilman
  -- strict thread matches above, loose matches on Subject: below --
2013-10-17 14:32 [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support Dave Martin
2013-10-17 16:04 ` Daniel Lezcano
2013-10-17 18:16   ` Nicolas Pitre
2013-10-17 15:46 Dave Martin
2013-10-18  8:29 ` Tarek Dakhran
2013-10-17 16:49 Dave Martin
2013-10-18 12:53 Dave Martin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).