| Message ID | 1405976886-27807-2-git-send-email-computersforpeace@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote: > +static DEFINE_SPINLOCK(boot_lock); > + > +static void brcmstb_secondary_init(unsigned int cpu) > +{ > + /* > + * Synchronise with the boot thread. > + */ > + spin_lock(&boot_lock); > + spin_unlock(&boot_lock); > +} > + > +static int brcmstb_boot_secondary(unsigned int cpu, struct task_struct *idle) > +{ > + /* > + * set synchronisation state between this boot processor > + * and the secondary one > + */ > + spin_lock(&boot_lock); > + > + /* Bring up power to the core if necessary */ > + if (brcmstb_cpu_get_power_state(cpu) == 0) > + brcmstb_cpu_power_on(cpu); > + > + brcmstb_cpu_boot(cpu); > + > + /* > + * now the secondary core is starting up let it run its > + * calibrations, then wait for it to finish > + */ > + spin_unlock(&boot_lock); I've just read through this code (because it caused my allmodconfig to break) and spotted this. What function does boot_lock perform here? Please, don't quote the comments (I know where the comments came from) but what I want to hear is your comments about why you decided to retain this.
On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris <computersforpeace@gmail.com> wrote: > From: Marc Carino <marc.ceeeee@gmail.com> > > The BCM7xxx series of Broadcom SoCs are used primarily in set-top boxes. > > This patch adds machine support for the ARM-based Broadcom SoCs. > > Signed-off-by: Marc Carino <marc.ceeeee@gmail.com> > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > arch/arm/configs/multi_v7_defconfig | 1 + > arch/arm/mach-bcm/Kconfig | 14 ++ > arch/arm/mach-bcm/Makefile | 5 + > arch/arm/mach-bcm/brcmstb.c | 28 +++ > arch/arm/mach-bcm/brcmstb.h | 19 ++ > arch/arm/mach-bcm/headsmp-brcmstb.S | 33 ++++ > arch/arm/mach-bcm/platsmp-brcmstb.c | 363 ++++++++++++++++++++++++++++++++++++ > 7 files changed, 463 insertions(+) > create mode 100644 arch/arm/mach-bcm/brcmstb.c > create mode 100644 arch/arm/mach-bcm/brcmstb.h > create mode 100644 arch/arm/mach-bcm/headsmp-brcmstb.S > create mode 100644 arch/arm/mach-bcm/platsmp-brcmstb.c > > diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig > index 534836497998..bf0df396a3cf 100644 > --- a/arch/arm/configs/multi_v7_defconfig > +++ b/arch/arm/configs/multi_v7_defconfig > @@ -19,6 +19,7 @@ CONFIG_MACH_DOVE=y > CONFIG_ARCH_BCM=y > CONFIG_ARCH_BCM_MOBILE=y > CONFIG_ARCH_BCM_5301X=y > +CONFIG_ARCH_BRCMSTB=y > CONFIG_ARCH_BERLIN=y > CONFIG_MACH_BERLIN_BG2=y > CONFIG_MACH_BERLIN_BG2CD=y > diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig > index 41c839167e87..0073633e7699 100644 > --- a/arch/arm/mach-bcm/Kconfig > +++ b/arch/arm/mach-bcm/Kconfig > @@ -87,4 +87,18 @@ config ARCH_BCM_5301X > different SoC or with the older BCM47XX and BCM53XX based > network SoC using a MIPS CPU, they are supported by arch/mips/bcm47xx > > +config ARCH_BRCMSTB > + bool "Broadcom BCM7XXX based boards" if ARCH_MULTI_V7 > + depends on MMU This was implied, but there are some patches from Arnd in this area. Does it really not build with !MMU? > + select ARM_GIC > + select MIGHT_HAVE_PCI > + select HAVE_SMP At least HAVE_SMP is for sure, but I think both of these are selected already . > + select HAVE_ARM_ARCH_TIMER > + help > + Say Y if you intend to run the kernel on a Broadcom ARM-based STB > + chipset. > + > + This enables support for Broadcom ARM-based set-top box chipsets, > + including the 7445 family of chips. > + > endif > diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile > index 731292114975..f3665121729b 100644 > --- a/arch/arm/mach-bcm/Makefile > +++ b/arch/arm/mach-bcm/Makefile > @@ -30,3 +30,8 @@ obj-$(CONFIG_ARCH_BCM2835) += board_bcm2835.o > > # BCM5301X > obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o > + > +ifeq ($(CONFIG_ARCH_BRCMSTB),y) > +obj-y += brcmstb.o > +obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o > +endif > diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c > new file mode 100644 > index 000000000000..60a5afa06ed7 > --- /dev/null > +++ b/arch/arm/mach-bcm/brcmstb.c > @@ -0,0 +1,28 @@ > +/* > + * Copyright (C) 2013-2014 Broadcom Corporation > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/init.h> > +#include <linux/of_platform.h> > + > +#include <asm/mach-types.h> > +#include <asm/mach/arch.h> > + > +static const char *brcmstb_match[] __initconst = { > + "brcm,bcm7445", > + "brcm,brcmstb", > + NULL > +}; > + > +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)") > + .dt_compat = brcmstb_match, > +MACHINE_END Do you plan to add more here? Otherwise you don't need this file. > diff --git a/arch/arm/mach-bcm/brcmstb.h b/arch/arm/mach-bcm/brcmstb.h > new file mode 100644 > index 000000000000..ec0c3d112b36 > --- /dev/null > +++ b/arch/arm/mach-bcm/brcmstb.h > @@ -0,0 +1,19 @@ > +/* > + * Copyright (C) 2013-2014 Broadcom Corporation > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef __BRCMSTB_H__ > +#define __BRCMSTB_H__ > + > +void brcmstb_secondary_startup(void); > + > +#endif /* __BRCMSTB_H__ */ > diff --git a/arch/arm/mach-bcm/headsmp-brcmstb.S b/arch/arm/mach-bcm/headsmp-brcmstb.S > new file mode 100644 > index 000000000000..199c1ea58248 > --- /dev/null > +++ b/arch/arm/mach-bcm/headsmp-brcmstb.S > @@ -0,0 +1,33 @@ > +/* > + * SMP boot code for secondary CPUs > + * Based on arch/arm/mach-tegra/headsmp.S > + * > + * Copyright (C) 2010 NVIDIA, Inc. > + * Copyright (C) 2013-2014 Broadcom Corporation > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <asm/assembler.h> > +#include <linux/linkage.h> > +#include <linux/init.h> > + > + .section ".text.head", "ax" > + > +ENTRY(brcmstb_secondary_startup) > + /* > + * Ensure CPU is in a sane state by disabling all IRQs and switching > + * into SVC mode. > + */ > + setmode PSR_I_BIT | PSR_F_BIT | SVC_MODE, r0 > + > + bl v7_invalidate_l1 This should be in your boot code. That is part of the documented entry requirements. Also, if you are coming straight from reset, there is other setup probably missing. Yes, there are others with this, but that doesn't make it right. > + b secondary_startup > +ENDPROC(brcmstb_secondary_startup) > diff --git a/arch/arm/mach-bcm/platsmp-brcmstb.c b/arch/arm/mach-bcm/platsmp-brcmstb.c > new file mode 100644 > index 000000000000..af780e9c23a6 > --- /dev/null > +++ b/arch/arm/mach-bcm/platsmp-brcmstb.c > @@ -0,0 +1,363 @@ > +/* > + * Broadcom STB CPU SMP and hotplug support for ARM > + * > + * Copyright (C) 2013-2014 Broadcom Corporation > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/delay.h> > +#include <linux/errno.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/of_address.h> > +#include <linux/of_platform.h> > +#include <linux/printk.h> > +#include <linux/regmap.h> > +#include <linux/smp.h> > +#include <linux/mfd/syscon.h> > +#include <linux/spinlock.h> > + > +#include <asm/cacheflush.h> > +#include <asm/cp15.h> > +#include <asm/mach-types.h> > +#include <asm/smp_plat.h> > + > +#include "brcmstb.h" > + > +enum { > + ZONE_MAN_CLKEN_MASK = BIT(0), > + ZONE_MAN_RESET_CNTL_MASK = BIT(1), > + ZONE_MAN_MEM_PWR_MASK = BIT(4), > + ZONE_RESERVED_1_MASK = BIT(5), > + ZONE_MAN_ISO_CNTL_MASK = BIT(6), > + ZONE_MANUAL_CONTROL_MASK = BIT(7), > + ZONE_PWR_DN_REQ_MASK = BIT(9), > + ZONE_PWR_UP_REQ_MASK = BIT(10), > + ZONE_BLK_RST_ASSERT_MASK = BIT(12), > + ZONE_PWR_OFF_STATE_MASK = BIT(25), > + ZONE_PWR_ON_STATE_MASK = BIT(26), > + ZONE_DPG_PWR_STATE_MASK = BIT(28), > + ZONE_MEM_PWR_STATE_MASK = BIT(29), > + ZONE_RESET_STATE_MASK = BIT(31), > + CPU0_PWR_ZONE_CTRL_REG = 1, > + CPU_RESET_CONFIG_REG = 2, > +}; > + > +static void __iomem *cpubiuctrl_block; > +static void __iomem *hif_cont_block; > +static u32 cpu0_pwr_zone_ctrl_reg; > +static u32 cpu_rst_cfg_reg; > +static u32 hif_cont_reg; > + > +#ifdef CONFIG_HOTPLUG_CPU > +static DEFINE_PER_CPU_ALIGNED(int, per_cpu_sw_state); > + > +static int per_cpu_sw_state_rd(u32 cpu) > +{ > + sync_cache_r(SHIFT_PERCPU_PTR(&per_cpu_sw_state, per_cpu_offset(cpu))); > + return per_cpu(per_cpu_sw_state, cpu); > +} > + > +static void per_cpu_sw_state_wr(u32 cpu, int val) > +{ > + per_cpu(per_cpu_sw_state, cpu) = val; > + dmb(); > + sync_cache_w(SHIFT_PERCPU_PTR(&per_cpu_sw_state, per_cpu_offset(cpu))); > + dsb_sev(); Barriers need to be documented why they are needed. > +} > +#else > +static inline void per_cpu_sw_state_wr(u32 cpu, int val) { } > +#endif > + > +static void __iomem *pwr_ctrl_get_base(u32 cpu) > +{ > + void __iomem *base = cpubiuctrl_block + cpu0_pwr_zone_ctrl_reg; > + base += (cpu_logical_map(cpu) * 4); > + return base; > +} > + > +static u32 pwr_ctrl_rd(u32 cpu) > +{ > + void __iomem *base = pwr_ctrl_get_base(cpu); > + return readl_relaxed(base); > +} > + > +static void pwr_ctrl_wr(u32 cpu, u32 val) > +{ > + void __iomem *base = pwr_ctrl_get_base(cpu); > + writel(val, base); > +} > + > +static void cpu_rst_cfg_set(u32 cpu, int set) > +{ > + u32 val; > + val = readl_relaxed(cpubiuctrl_block + cpu_rst_cfg_reg); > + if (set) > + val |= BIT(cpu_logical_map(cpu)); > + else > + val &= ~BIT(cpu_logical_map(cpu)); > + writel_relaxed(val, cpubiuctrl_block + cpu_rst_cfg_reg); > +} > + > +static void cpu_set_boot_addr(u32 cpu, unsigned long boot_addr) > +{ > + const int reg_ofs = cpu_logical_map(cpu) * 8; > + writel_relaxed(0, hif_cont_block + hif_cont_reg + reg_ofs); > + writel_relaxed(boot_addr, hif_cont_block + hif_cont_reg + 4 + reg_ofs); > +} > + > +static void brcmstb_cpu_boot(u32 cpu) > +{ > + pr_info("SMP: Booting CPU%d...\n", cpu); The core already prints messages. > + > + /* > + * set the reset vector to point to the secondary_startup > + * routine > + */ > + cpu_set_boot_addr(cpu, virt_to_phys(brcmstb_secondary_startup)); > + > + /* unhalt the cpu */ > + cpu_rst_cfg_set(cpu, 0); > +} > + > +static void brcmstb_cpu_power_on(u32 cpu) > +{ > + /* > + * The secondary cores power was cut, so we must go through > + * power-on initialization. > + */ > + u32 tmp; > + > + pr_info("SMP: Powering up CPU%d...\n", cpu); ditto. > + > + /* Request zone power up */ > + pwr_ctrl_wr(cpu, ZONE_PWR_UP_REQ_MASK); > + > + /* Wait for the power up FSM to complete */ > + do { > + tmp = pwr_ctrl_rd(cpu); > + } while (!(tmp & ZONE_PWR_ON_STATE_MASK)); Perhaps a timeout? Or perhaps you don't need to wait here. The core code will wait for the core to come online. > + > + per_cpu_sw_state_wr(cpu, 1); The kernel already tracks the state. > +} > + > +static int brcmstb_cpu_get_power_state(u32 cpu) > +{ > + int tmp = pwr_ctrl_rd(cpu); > + return (tmp & ZONE_RESET_STATE_MASK) ? 0 : 1; > +} > + > +#ifdef CONFIG_HOTPLUG_CPU > + > +static void brcmstb_cpu_die(u32 cpu) > +{ > + v7_exit_coherency_flush(all); > + > + /* Prevent all interrupts from reaching this CPU. */ > + arch_local_irq_disable(); Surely that is already done? And disabling IRQ has no effect on wake-up from WFI. > + > + /* > + * Final full barrier to ensure everything before this instruction has > + * quiesced. > + */ > + isb(); > + dsb(); > + > + per_cpu_sw_state_wr(cpu, 0); > + > + /* Sit and wait to die */ > + wfi(); > + > + /* We should never get here... */ > + panic("Spurious interrupt on CPU %d received!\n", cpu); > +} > + > +static int brcmstb_cpu_kill(u32 cpu) > +{ > + u32 tmp; > + > + pr_info("SMP: Powering down CPU%d...\n", cpu); > + > + while (per_cpu_sw_state_rd(cpu)) > + ; I don't think this is necessary. You don't need to synchronize die and kill. Look at other implementations that actually power down the core (vs. just wfi). > + > + /* Program zone reset */ > + pwr_ctrl_wr(cpu, ZONE_RESET_STATE_MASK | ZONE_BLK_RST_ASSERT_MASK | > + ZONE_PWR_DN_REQ_MASK); > + > + /* Verify zone reset */ > + tmp = pwr_ctrl_rd(cpu); > + if (!(tmp & ZONE_RESET_STATE_MASK)) > + pr_err("%s: Zone reset bit for CPU %d not asserted!\n", > + __func__, cpu); > + > + /* Wait for power down */ > + do { > + tmp = pwr_ctrl_rd(cpu); > + } while (!(tmp & ZONE_PWR_OFF_STATE_MASK)); > + > + /* Settle-time from Broadcom-internal DVT reference code */ > + udelay(7); > + > + /* Assert reset on the CPU */ > + cpu_rst_cfg_set(cpu, 1); > + > + return 1; > +} > + > +#endif /* CONFIG_HOTPLUG_CPU */ > + > +static int __init setup_hifcpubiuctrl_regs(struct device_node *np) > +{ > + int rc = 0; > + char *name; > + struct device_node *syscon_np = NULL; > + > + name = "syscon-cpu"; > + > + syscon_np = of_parse_phandle(np, name, 0); > + if (!syscon_np) { > + pr_err("can't find phandle %s\n", name); > + rc = -EINVAL; > + goto cleanup; > + } > + > + cpubiuctrl_block = of_iomap(syscon_np, 0); > + if (!cpubiuctrl_block) { > + pr_err("iomap failed for cpubiuctrl_block\n"); > + rc = -EINVAL; > + goto cleanup; > + } > + > + rc = of_property_read_u32_index(np, name, CPU0_PWR_ZONE_CTRL_REG, > + &cpu0_pwr_zone_ctrl_reg); > + if (rc) { > + pr_err("failed to read 1st entry from %s property (%d)\n", name, > + rc); > + rc = -EINVAL; > + goto cleanup; > + } > + > + rc = of_property_read_u32_index(np, name, CPU_RESET_CONFIG_REG, > + &cpu_rst_cfg_reg); > + if (rc) { > + pr_err("failed to read 2nd entry from %s property (%d)\n", name, > + rc); > + rc = -EINVAL; > + goto cleanup; > + } > + > +cleanup: > + if (syscon_np) > + of_node_put(syscon_np); > + > + return rc; > +} > + > +static int __init setup_hifcont_regs(struct device_node *np) > +{ > + int rc = 0; > + char *name; > + struct device_node *syscon_np = NULL; > + > + name = "syscon-cont"; > + > + syscon_np = of_parse_phandle(np, name, 0); > + if (!syscon_np) { > + pr_err("can't find phandle %s\n", name); > + rc = -EINVAL; > + goto cleanup; > + } > + > + hif_cont_block = of_iomap(syscon_np, 0); > + if (!hif_cont_block) { > + pr_err("iomap failed for hif_cont_block\n"); > + rc = -EINVAL; > + goto cleanup; > + } > + > + /* offset is at top of hif_cont_block */ > + hif_cont_reg = 0; > + > +cleanup: > + if (syscon_np) > + of_node_put(syscon_np); > + > + return rc; > +} > + > +static void __init brcmstb_cpu_ctrl_setup(unsigned int max_cpus) > +{ > + int rc; > + struct device_node *np; > + char *name; > + > + name = "brcm,brcmstb-smpboot"; > + np = of_find_compatible_node(NULL, NULL, name); > + if (!np) { > + pr_err("can't find compatible node %s\n", name); > + return; > + } > + > + rc = setup_hifcpubiuctrl_regs(np); > + if (rc) > + return; > + > + rc = setup_hifcont_regs(np); > + if (rc) > + return; > +} > + > +static DEFINE_SPINLOCK(boot_lock); > + > +static void brcmstb_secondary_init(unsigned int cpu) > +{ > + /* > + * Synchronise with the boot thread. > + */ > + spin_lock(&boot_lock); > + spin_unlock(&boot_lock); I suggest you read previous discussions on attempts to make this common before replying to Russell. > +} > + > +static int brcmstb_boot_secondary(unsigned int cpu, struct task_struct *idle) > +{ > + /* > + * set synchronisation state between this boot processor > + * and the secondary one > + */ > + spin_lock(&boot_lock); > + > + /* Bring up power to the core if necessary */ > + if (brcmstb_cpu_get_power_state(cpu) == 0) > + brcmstb_cpu_power_on(cpu); > + > + brcmstb_cpu_boot(cpu); > + > + /* > + * now the secondary core is starting up let it run its > + * calibrations, then wait for it to finish > + */ > + spin_unlock(&boot_lock); > + > + return 0; > +} > + > +static struct smp_operations brcmstb_smp_ops __initdata = { > + .smp_prepare_cpus = brcmstb_cpu_ctrl_setup, > + .smp_secondary_init = brcmstb_secondary_init, > + .smp_boot_secondary = brcmstb_boot_secondary, > +#ifdef CONFIG_HOTPLUG_CPU > + .cpu_kill = brcmstb_cpu_kill, > + .cpu_die = brcmstb_cpu_die, > +#endif > +}; > + > +CPU_METHOD_OF_DECLARE(brcmstb_smp, "brcm,brahma-b15", &brcmstb_smp_ops); > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Rob, I appreciate your comments, but where were many of these 5 months ago on the first 7 revisions? :) On a practical note: v9 is already queued for 3.17. Should I send patches for the 3.17 cycle (or later) to fixup some of these issues? Or would you recommend pulling the patches out of Matt Porter's tree now, and reintroducing for 3.18? (I would be much happier with the first.) I do note that we at least need to fix allmodconfig ASAP; I'll reply to Russell on that one. On Wed, Jul 30, 2014 at 12:09:48PM -0500, Rob Herring wrote: > On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris <computersforpeace@gmail.com> wrote: > > From: Marc Carino <marc.ceeeee@gmail.com> > > > > The BCM7xxx series of Broadcom SoCs are used primarily in set-top boxes. > > > > This patch adds machine support for the ARM-based Broadcom SoCs. > > > > Signed-off-by: Marc Carino <marc.ceeeee@gmail.com> > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > --- > > arch/arm/configs/multi_v7_defconfig | 1 + > > arch/arm/mach-bcm/Kconfig | 14 ++ > > arch/arm/mach-bcm/Makefile | 5 + > > arch/arm/mach-bcm/brcmstb.c | 28 +++ > > arch/arm/mach-bcm/brcmstb.h | 19 ++ > > arch/arm/mach-bcm/headsmp-brcmstb.S | 33 ++++ > > arch/arm/mach-bcm/platsmp-brcmstb.c | 363 ++++++++++++++++++++++++++++++++++++ > > 7 files changed, 463 insertions(+) > > create mode 100644 arch/arm/mach-bcm/brcmstb.c > > create mode 100644 arch/arm/mach-bcm/brcmstb.h > > create mode 100644 arch/arm/mach-bcm/headsmp-brcmstb.S > > create mode 100644 arch/arm/mach-bcm/platsmp-brcmstb.c > > > > diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig > > index 534836497998..bf0df396a3cf 100644 > > --- a/arch/arm/configs/multi_v7_defconfig > > +++ b/arch/arm/configs/multi_v7_defconfig > > @@ -19,6 +19,7 @@ CONFIG_MACH_DOVE=y > > CONFIG_ARCH_BCM=y > > CONFIG_ARCH_BCM_MOBILE=y > > CONFIG_ARCH_BCM_5301X=y > > +CONFIG_ARCH_BRCMSTB=y > > CONFIG_ARCH_BERLIN=y > > CONFIG_MACH_BERLIN_BG2=y > > CONFIG_MACH_BERLIN_BG2CD=y > > diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig > > index 41c839167e87..0073633e7699 100644 > > --- a/arch/arm/mach-bcm/Kconfig > > +++ b/arch/arm/mach-bcm/Kconfig > > @@ -87,4 +87,18 @@ config ARCH_BCM_5301X > > different SoC or with the older BCM47XX and BCM53XX based > > network SoC using a MIPS CPU, they are supported by arch/mips/bcm47xx > > > > +config ARCH_BRCMSTB > > + bool "Broadcom BCM7XXX based boards" if ARCH_MULTI_V7 > > + depends on MMU > > This was implied, but there are some patches from Arnd in this area. > Does it really not build with !MMU? I suppose it probably builds, it likely won't run. I can drop this line and then reassess if ARCH_MULTIPLATFORM becomes buildable with !MMU. > > + select ARM_GIC > > > + select MIGHT_HAVE_PCI > > + select HAVE_SMP > > At least HAVE_SMP is for sure, but I think both of these are selected already . You're correct. ARCH_MULTIPLATFORM selects MIGHT_HAVE_PCI and ARCH_MULTI_V7 selects HAVE_SMP. I can look at dropping these. > > + select HAVE_ARM_ARCH_TIMER > > + help > > + Say Y if you intend to run the kernel on a Broadcom ARM-based STB > > + chipset. > > + > > + This enables support for Broadcom ARM-based set-top box chipsets, > > + including the 7445 family of chips. > > + > > endif > > diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile > > index 731292114975..f3665121729b 100644 > > --- a/arch/arm/mach-bcm/Makefile > > +++ b/arch/arm/mach-bcm/Makefile > > @@ -30,3 +30,8 @@ obj-$(CONFIG_ARCH_BCM2835) += board_bcm2835.o > > > > # BCM5301X > > obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o > > + > > +ifeq ($(CONFIG_ARCH_BRCMSTB),y) > > +obj-y += brcmstb.o > > +obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o > > +endif > > diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c > > new file mode 100644 > > index 000000000000..60a5afa06ed7 > > --- /dev/null > > +++ b/arch/arm/mach-bcm/brcmstb.c > > @@ -0,0 +1,28 @@ > > +/* > > + * Copyright (C) 2013-2014 Broadcom Corporation > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/of_platform.h> > > + > > +#include <asm/mach-types.h> > > +#include <asm/mach/arch.h> > > + > > +static const char *brcmstb_match[] __initconst = { > > + "brcm,bcm7445", > > + "brcm,brcmstb", > > + NULL > > +}; > > + > > +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)") > > + .dt_compat = brcmstb_match, > > +MACHINE_END > > Do you plan to add more here? Otherwise you don't need this file. Probably eventually, but not yet (there's out-of-tree stuff that hasn't been integrated); should we drop the file until it's needed? (Over 9 revisions, this file has evolved. And the CPU_METHOD_OF_DECLARE stuff appearing in the meantime, which cut this file down to a no-op.) > > diff --git a/arch/arm/mach-bcm/brcmstb.h b/arch/arm/mach-bcm/brcmstb.h > > new file mode 100644 > > index 000000000000..ec0c3d112b36 > > --- /dev/null > > +++ b/arch/arm/mach-bcm/brcmstb.h > > @@ -0,0 +1,19 @@ > > +/* > > + * Copyright (C) 2013-2014 Broadcom Corporation > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#ifndef __BRCMSTB_H__ > > +#define __BRCMSTB_H__ > > + > > +void brcmstb_secondary_startup(void); > > + > > +#endif /* __BRCMSTB_H__ */ > > diff --git a/arch/arm/mach-bcm/headsmp-brcmstb.S b/arch/arm/mach-bcm/headsmp-brcmstb.S > > new file mode 100644 > > index 000000000000..199c1ea58248 > > --- /dev/null > > +++ b/arch/arm/mach-bcm/headsmp-brcmstb.S > > @@ -0,0 +1,33 @@ > > +/* > > + * SMP boot code for secondary CPUs > > + * Based on arch/arm/mach-tegra/headsmp.S > > + * > > + * Copyright (C) 2010 NVIDIA, Inc. > > + * Copyright (C) 2013-2014 Broadcom Corporation > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <asm/assembler.h> > > +#include <linux/linkage.h> > > +#include <linux/init.h> > > + > > + .section ".text.head", "ax" > > + > > +ENTRY(brcmstb_secondary_startup) > > + /* > > + * Ensure CPU is in a sane state by disabling all IRQs and switching > > + * into SVC mode. > > + */ > > + setmode PSR_I_BIT | PSR_F_BIT | SVC_MODE, r0 > > + > > + bl v7_invalidate_l1 > > This should be in your boot code. That is part of the documented entry > requirements. By "documented" are you referring to the ARM TRM? Wouldn't any "boot code" requirements only apply to CPU0? Linux prepares the non-boot CPUs for SMP, as you see here. > Also, if you are coming straight from reset, there is > other setup probably missing. Like what? > Yes, there are others with this, but that doesn't make it right. > > > + b secondary_startup > > +ENDPROC(brcmstb_secondary_startup) > > diff --git a/arch/arm/mach-bcm/platsmp-brcmstb.c b/arch/arm/mach-bcm/platsmp-brcmstb.c > > new file mode 100644 > > index 000000000000..af780e9c23a6 > > --- /dev/null > > +++ b/arch/arm/mach-bcm/platsmp-brcmstb.c > > @@ -0,0 +1,363 @@ > > +/* > > + * Broadcom STB CPU SMP and hotplug support for ARM > > + * > > + * Copyright (C) 2013-2014 Broadcom Corporation > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <linux/delay.h> > > +#include <linux/errno.h> > > +#include <linux/init.h> > > +#include <linux/io.h> > > +#include <linux/of_address.h> > > +#include <linux/of_platform.h> > > +#include <linux/printk.h> > > +#include <linux/regmap.h> > > +#include <linux/smp.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/spinlock.h> > > + > > +#include <asm/cacheflush.h> > > +#include <asm/cp15.h> > > +#include <asm/mach-types.h> > > +#include <asm/smp_plat.h> > > + > > +#include "brcmstb.h" > > + > > +enum { > > + ZONE_MAN_CLKEN_MASK = BIT(0), > > + ZONE_MAN_RESET_CNTL_MASK = BIT(1), > > + ZONE_MAN_MEM_PWR_MASK = BIT(4), > > + ZONE_RESERVED_1_MASK = BIT(5), > > + ZONE_MAN_ISO_CNTL_MASK = BIT(6), > > + ZONE_MANUAL_CONTROL_MASK = BIT(7), > > + ZONE_PWR_DN_REQ_MASK = BIT(9), > > + ZONE_PWR_UP_REQ_MASK = BIT(10), > > + ZONE_BLK_RST_ASSERT_MASK = BIT(12), > > + ZONE_PWR_OFF_STATE_MASK = BIT(25), > > + ZONE_PWR_ON_STATE_MASK = BIT(26), > > + ZONE_DPG_PWR_STATE_MASK = BIT(28), > > + ZONE_MEM_PWR_STATE_MASK = BIT(29), > > + ZONE_RESET_STATE_MASK = BIT(31), > > + CPU0_PWR_ZONE_CTRL_REG = 1, > > + CPU_RESET_CONFIG_REG = 2, > > +}; > > + > > +static void __iomem *cpubiuctrl_block; > > +static void __iomem *hif_cont_block; > > +static u32 cpu0_pwr_zone_ctrl_reg; > > +static u32 cpu_rst_cfg_reg; > > +static u32 hif_cont_reg; > > + > > +#ifdef CONFIG_HOTPLUG_CPU > > +static DEFINE_PER_CPU_ALIGNED(int, per_cpu_sw_state); > > + > > +static int per_cpu_sw_state_rd(u32 cpu) > > +{ > > + sync_cache_r(SHIFT_PERCPU_PTR(&per_cpu_sw_state, per_cpu_offset(cpu))); > > + return per_cpu(per_cpu_sw_state, cpu); > > +} > > + > > +static void per_cpu_sw_state_wr(u32 cpu, int val) > > +{ > > + per_cpu(per_cpu_sw_state, cpu) = val; > > + dmb(); > > + sync_cache_w(SHIFT_PERCPU_PTR(&per_cpu_sw_state, per_cpu_offset(cpu))); > > + dsb_sev(); > > Barriers need to be documented why they are needed. (NB: I see approximately 3 total existing cases where there is a comment near a dsb()/dsb_sev().) This was actually adapted from the mcpm code, and (1) I think the dmb() is in the wrong place (it should be the first thing in this function); (2) the SEV is unnecessary (we don't use WFE; just a cache-invalidated polling loop); and (3) the DSB is also unnecessary, since sync_cache_w() handles its own barriers. Plus, we should probably add some comments to describe what's going on here. (Follow-up patch?) > > +} > > +#else > > +static inline void per_cpu_sw_state_wr(u32 cpu, int val) { } > > +#endif > > + > > +static void __iomem *pwr_ctrl_get_base(u32 cpu) > > +{ > > + void __iomem *base = cpubiuctrl_block + cpu0_pwr_zone_ctrl_reg; > > + base += (cpu_logical_map(cpu) * 4); > > + return base; > > +} > > + > > +static u32 pwr_ctrl_rd(u32 cpu) > > +{ > > + void __iomem *base = pwr_ctrl_get_base(cpu); > > + return readl_relaxed(base); > > +} > > + > > +static void pwr_ctrl_wr(u32 cpu, u32 val) > > +{ > > + void __iomem *base = pwr_ctrl_get_base(cpu); > > + writel(val, base); > > +} > > + > > +static void cpu_rst_cfg_set(u32 cpu, int set) > > +{ > > + u32 val; > > + val = readl_relaxed(cpubiuctrl_block + cpu_rst_cfg_reg); > > + if (set) > > + val |= BIT(cpu_logical_map(cpu)); > > + else > > + val &= ~BIT(cpu_logical_map(cpu)); > > + writel_relaxed(val, cpubiuctrl_block + cpu_rst_cfg_reg); > > +} > > + > > +static void cpu_set_boot_addr(u32 cpu, unsigned long boot_addr) > > +{ > > + const int reg_ofs = cpu_logical_map(cpu) * 8; > > + writel_relaxed(0, hif_cont_block + hif_cont_reg + reg_ofs); > > + writel_relaxed(boot_addr, hif_cont_block + hif_cont_reg + 4 + reg_ofs); > > +} > > + > > +static void brcmstb_cpu_boot(u32 cpu) > > +{ > > + pr_info("SMP: Booting CPU%d...\n", cpu); > > The core already prints messages. Ok, we can drop. > > + > > + /* > > + * set the reset vector to point to the secondary_startup > > + * routine > > + */ > > + cpu_set_boot_addr(cpu, virt_to_phys(brcmstb_secondary_startup)); > > + > > + /* unhalt the cpu */ > > + cpu_rst_cfg_set(cpu, 0); > > +} > > + > > +static void brcmstb_cpu_power_on(u32 cpu) > > +{ > > + /* > > + * The secondary cores power was cut, so we must go through > > + * power-on initialization. > > + */ > > + u32 tmp; > > + > > + pr_info("SMP: Powering up CPU%d...\n", cpu); > > ditto. > > > + > > + /* Request zone power up */ > > + pwr_ctrl_wr(cpu, ZONE_PWR_UP_REQ_MASK); > > + > > + /* Wait for the power up FSM to complete */ > > + do { > > + tmp = pwr_ctrl_rd(cpu); > > + } while (!(tmp & ZONE_PWR_ON_STATE_MASK)); > > Perhaps a timeout? Or perhaps you don't need to wait here. The core > code will wait for the core to come online. I believe we do have to wait for power before we can proceed to poke the core. This is a quirk of our chip, explained below. But sure, I could try to add a timeout. > > + > > + per_cpu_sw_state_wr(cpu, 1); > > The kernel already tracks the state. Yes, but it doesn't synchronize cpu_die() and cpu_kill(). But I see you objected below. > > +} > > + > > +static int brcmstb_cpu_get_power_state(u32 cpu) > > +{ > > + int tmp = pwr_ctrl_rd(cpu); > > + return (tmp & ZONE_RESET_STATE_MASK) ? 0 : 1; > > +} > > + > > +#ifdef CONFIG_HOTPLUG_CPU > > + > > +static void brcmstb_cpu_die(u32 cpu) > > +{ > > + v7_exit_coherency_flush(all); > > + > > + /* Prevent all interrupts from reaching this CPU. */ > > + arch_local_irq_disable(); > > Surely that is already done? True. Third line in arch/arm/kernel/smp.c cpu_die(). I believe we can drop this. > And disabling IRQ has no effect on > wake-up from WFI. > > > + > > + /* > > + * Final full barrier to ensure everything before this instruction has > > + * quiesced. > > + */ > > + isb(); > > + dsb(); > > + > > + per_cpu_sw_state_wr(cpu, 0); > > + > > + /* Sit and wait to die */ > > + wfi(); > > + > > + /* We should never get here... */ > > + panic("Spurious interrupt on CPU %d received!\n", cpu); > > +} > > + > > +static int brcmstb_cpu_kill(u32 cpu) > > +{ > > + u32 tmp; > > + > > + pr_info("SMP: Powering down CPU%d...\n", cpu); > > + > > + while (per_cpu_sw_state_rd(cpu)) > > + ; > > I don't think this is necessary. You don't need to synchronize die and > kill. Look at other implementations that actually power down the core > (vs. just wfi). Hmm, I'm pretty sure the synchronization is required for our B15 core. If we yank the power before the core has properly quiesced, the whole CPU complex will lock up. (Similar story for the power-up while loop you complained about above.) > > + > > + /* Program zone reset */ > > + pwr_ctrl_wr(cpu, ZONE_RESET_STATE_MASK | ZONE_BLK_RST_ASSERT_MASK | > > + ZONE_PWR_DN_REQ_MASK); > > + > > + /* Verify zone reset */ > > + tmp = pwr_ctrl_rd(cpu); > > + if (!(tmp & ZONE_RESET_STATE_MASK)) > > + pr_err("%s: Zone reset bit for CPU %d not asserted!\n", > > + __func__, cpu); > > + > > + /* Wait for power down */ > > + do { > > + tmp = pwr_ctrl_rd(cpu); > > + } while (!(tmp & ZONE_PWR_OFF_STATE_MASK)); > > + > > + /* Settle-time from Broadcom-internal DVT reference code */ > > + udelay(7); > > + > > + /* Assert reset on the CPU */ > > + cpu_rst_cfg_set(cpu, 1); > > + > > + return 1; > > +} > > + > > +#endif /* CONFIG_HOTPLUG_CPU */ > > + > > +static int __init setup_hifcpubiuctrl_regs(struct device_node *np) > > +{ > > + int rc = 0; > > + char *name; > > + struct device_node *syscon_np = NULL; > > + > > + name = "syscon-cpu"; > > + > > + syscon_np = of_parse_phandle(np, name, 0); > > + if (!syscon_np) { > > + pr_err("can't find phandle %s\n", name); > > + rc = -EINVAL; > > + goto cleanup; > > + } > > + > > + cpubiuctrl_block = of_iomap(syscon_np, 0); > > + if (!cpubiuctrl_block) { > > + pr_err("iomap failed for cpubiuctrl_block\n"); > > + rc = -EINVAL; > > + goto cleanup; > > + } > > + > > + rc = of_property_read_u32_index(np, name, CPU0_PWR_ZONE_CTRL_REG, > > + &cpu0_pwr_zone_ctrl_reg); > > + if (rc) { > > + pr_err("failed to read 1st entry from %s property (%d)\n", name, > > + rc); > > + rc = -EINVAL; > > + goto cleanup; > > + } > > + > > + rc = of_property_read_u32_index(np, name, CPU_RESET_CONFIG_REG, > > + &cpu_rst_cfg_reg); > > + if (rc) { > > + pr_err("failed to read 2nd entry from %s property (%d)\n", name, > > + rc); > > + rc = -EINVAL; > > + goto cleanup; > > + } > > + > > +cleanup: > > + if (syscon_np) > > + of_node_put(syscon_np); > > + > > + return rc; > > +} > > + > > +static int __init setup_hifcont_regs(struct device_node *np) > > +{ > > + int rc = 0; > > + char *name; > > + struct device_node *syscon_np = NULL; > > + > > + name = "syscon-cont"; > > + > > + syscon_np = of_parse_phandle(np, name, 0); > > + if (!syscon_np) { > > + pr_err("can't find phandle %s\n", name); > > + rc = -EINVAL; > > + goto cleanup; > > + } > > + > > + hif_cont_block = of_iomap(syscon_np, 0); > > + if (!hif_cont_block) { > > + pr_err("iomap failed for hif_cont_block\n"); > > + rc = -EINVAL; > > + goto cleanup; > > + } > > + > > + /* offset is at top of hif_cont_block */ > > + hif_cont_reg = 0; > > + > > +cleanup: > > + if (syscon_np) > > + of_node_put(syscon_np); > > + > > + return rc; > > +} > > + > > +static void __init brcmstb_cpu_ctrl_setup(unsigned int max_cpus) > > +{ > > + int rc; > > + struct device_node *np; > > + char *name; > > + > > + name = "brcm,brcmstb-smpboot"; > > + np = of_find_compatible_node(NULL, NULL, name); > > + if (!np) { > > + pr_err("can't find compatible node %s\n", name); > > + return; > > + } > > + > > + rc = setup_hifcpubiuctrl_regs(np); > > + if (rc) > > + return; > > + > > + rc = setup_hifcont_regs(np); > > + if (rc) > > + return; > > +} > > + > > +static DEFINE_SPINLOCK(boot_lock); > > + > > +static void brcmstb_secondary_init(unsigned int cpu) > > +{ > > + /* > > + * Synchronise with the boot thread. > > + */ > > + spin_lock(&boot_lock); > > + spin_unlock(&boot_lock); > > I suggest you read previous discussions on attempts to make this > common before replying to Russell. Can you point me to this discussion? linux-arm-kernel is a big and noisy world... BTW, I'm not sure the boot_lock is actually necessary at all for us; it was just borrowed from one of the other mach-*'s. The synchronization we need is done in the while loops above. > > +} > > + > > +static int brcmstb_boot_secondary(unsigned int cpu, struct task_struct *idle) > > +{ > > + /* > > + * set synchronisation state between this boot processor > > + * and the secondary one > > + */ > > + spin_lock(&boot_lock); > > + > > + /* Bring up power to the core if necessary */ > > + if (brcmstb_cpu_get_power_state(cpu) == 0) > > + brcmstb_cpu_power_on(cpu); > > + > > + brcmstb_cpu_boot(cpu); > > + > > + /* > > + * now the secondary core is starting up let it run its > > + * calibrations, then wait for it to finish > > + */ > > + spin_unlock(&boot_lock); > > + > > + return 0; > > +} > > + > > +static struct smp_operations brcmstb_smp_ops __initdata = { > > + .smp_prepare_cpus = brcmstb_cpu_ctrl_setup, > > + .smp_secondary_init = brcmstb_secondary_init, > > + .smp_boot_secondary = brcmstb_boot_secondary, > > +#ifdef CONFIG_HOTPLUG_CPU > > + .cpu_kill = brcmstb_cpu_kill, > > + .cpu_die = brcmstb_cpu_die, > > +#endif > > +}; > > + > > +CPU_METHOD_OF_DECLARE(brcmstb_smp, "brcm,brahma-b15", &brcmstb_smp_ops); Brian
On Wed, Jul 30, 2014 at 07:23:20PM -0700, Brian Norris wrote: > Hi Rob, > > I appreciate your comments, but where were many of these 5 months ago on > the first 7 revisions? :) > > On a practical note: v9 is already queued for 3.17. Should I send > patches for the 3.17 cycle (or later) to fixup some of these issues? Or > would you recommend pulling the patches out of Matt Porter's tree now, > and reintroducing for 3.18? (I would be much happier with the first.) > > I do note that we at least need to fix allmodconfig ASAP; I'll reply to > Russell on that one. > > On Wed, Jul 30, 2014 at 12:09:48PM -0500, Rob Herring wrote: > > On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris <computersforpeace@gmail.com> wrote: > > > + > > > + per_cpu_sw_state_wr(cpu, 1); > > > > The kernel already tracks the state. > > Yes, but it doesn't synchronize cpu_die() and cpu_kill(). But I see you > objected below. Err. static DECLARE_COMPLETION(cpu_died); /* * called on the thread which is asking for a CPU to be shutdown - * waits until shutdown has completed, or it is timed out. */ void __cpu_die(unsigned int cpu) { if (!wait_for_completion_timeout(&cpu_died, msecs_to_jiffies(5000))) { pr_err("CPU%u: cpu didn't die\n", cpu); return; } printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); ... if (!platform_cpu_kill(cpu)) printk("CPU%u: unable to kill\n", cpu); } void __ref cpu_die(void) { ... /* * Tell __cpu_die() that this CPU is now safe to dispose of. Once * this returns, power and/or clocks can be removed at any point * from this CPU and its cache by platform_cpu_kill(). */ complete(&cpu_died); There _is_ synchronisation between these two. Your cpu_kill() function will not be called until we have flushed all data from the dying CPU's cache. That's the best we can do, because if we cause the dying CPU to exit the coherency domain, any kind of locking or completion will no longer work (neither will your state tracking solution because the L1 caches will no longer snoop.) > Hmm, I'm pretty sure the synchronization is required for our B15 core. > If we yank the power before the core has properly quiesced, the whole > CPU complex will lock up. (Similar story for the power-up while loop you > complained about above.) If you need to ensure that the power isn't turned off too soon, how about using a delayed work queue to do the power-off of the CPU (remembering to cancel the delayed work queue when powering the CPU back up.)
Hi Russell, On Thu, Jul 31, 2014 at 09:43:15AM +0100, Russell King wrote: > On Wed, Jul 30, 2014 at 07:23:20PM -0700, Brian Norris wrote: > > I appreciate your comments, but where were many of these 5 months ago on > > the first 7 revisions? :) > > > > On a practical note: v9 is already queued for 3.17. Should I send > > patches for the 3.17 cycle (or later) to fixup some of these issues? Or > > would you recommend pulling the patches out of Matt Porter's tree now, > > and reintroducing for 3.18? (I would be much happier with the first.) > > > > I do note that we at least need to fix allmodconfig ASAP; I'll reply to > > Russell on that one. > > > > On Wed, Jul 30, 2014 at 12:09:48PM -0500, Rob Herring wrote: > > > On Mon, Jul 21, 2014 at 4:07 PM, Brian Norris <computersforpeace@gmail.com> wrote: > > > > + > > > > + per_cpu_sw_state_wr(cpu, 1); > > > > > > The kernel already tracks the state. > > > > Yes, but it doesn't synchronize cpu_die() and cpu_kill(). But I see you > > objected below. > > Err. > > static DECLARE_COMPLETION(cpu_died); Yes, I noticed this. What I meant is that smp_ops.cpu_die() and smp_ops.cpu_kill() are not synchronized. > /* > * called on the thread which is asking for a CPU to be shutdown - > * waits until shutdown has completed, or it is timed out. > */ > void __cpu_die(unsigned int cpu) > { > if (!wait_for_completion_timeout(&cpu_died, msecs_to_jiffies(5000))) { > pr_err("CPU%u: cpu didn't die\n", cpu); > return; > } > printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); > ... > if (!platform_cpu_kill(cpu)) > printk("CPU%u: unable to kill\n", cpu); > } > > void __ref cpu_die(void) > { > ... > /* > * Tell __cpu_die() that this CPU is now safe to dispose of. Once > * this returns, power and/or clocks can be removed at any point > * from this CPU and its cache by platform_cpu_kill(). > */ > complete(&cpu_died); > > There _is_ synchronisation between these two. Your cpu_kill() function > will not be called until we have flushed all data from the dying CPU's > cache. That's the best we can do, because if we cause the dying CPU to > exit the coherency domain, any kind of locking or completion will no > longer work (neither will your state tracking solution because the L1 > caches will no longer snoop.) We're not relying on the L1 cache, though. Don't sync_cache_{r,w}() ensure all reads/writes reach at least the L2? Besides, we modeled this after the MCPM code (e.g., __mcpm_cpu_down()). Are both implementations incorrect? > > Hmm, I'm pretty sure the synchronization is required for our B15 core. > > If we yank the power before the core has properly quiesced, the whole > > CPU complex will lock up. (Similar story for the power-up while loop you > > complained about above.) > > If you need to ensure that the power isn't turned off too soon, how about > using a delayed work queue to do the power-off of the CPU (remembering to > cancel the delayed work queue when powering the CPU back up.) How does that ensure that the CPU is down by the time the work is scheduled? It seems like this would just defer the work long enough that it *most likely* has quiesced, but I don't see how this gives us a better guarantee. Or maybe I'm missing something. (If so, please do enlighten!) Brian
On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris <computersforpeace@gmail.com> wrote: > Hi Rob, > > I appreciate your comments, but where were many of these 5 months ago on > the first 7 revisions? :) Sorry, but that is the nature of upstreaming. But given some of the issues, it is obvious the reviews were not sufficient. > On a practical note: v9 is already queued for 3.17. Should I send > patches for the 3.17 cycle (or later) to fixup some of these issues? Or > would you recommend pulling the patches out of Matt Porter's tree now, > and reintroducing for 3.18? (I would be much happier with the first.) Things can always be un-queued. I guess that's Matt's and arm-soc's decision. >> > +static const char *brcmstb_match[] __initconst = { >> > + "brcm,bcm7445", >> > + "brcm,brcmstb", >> > + NULL >> > +}; >> > + >> > +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)") >> > + .dt_compat = brcmstb_match, >> > +MACHINE_END >> >> Do you plan to add more here? Otherwise you don't need this file. > > Probably eventually, but not yet (there's out-of-tree stuff that hasn't > been integrated); should we drop the file until it's needed? I would say yes if you re-spin the patches. >> > +ENTRY(brcmstb_secondary_startup) >> > + /* >> > + * Ensure CPU is in a sane state by disabling all IRQs and switching >> > + * into SVC mode. >> > + */ >> > + setmode PSR_I_BIT | PSR_F_BIT | SVC_MODE, r0 >> > + >> > + bl v7_invalidate_l1 >> >> This should be in your boot code. That is part of the documented entry >> requirements. > > By "documented" are you referring to the ARM TRM? Wouldn't any "boot > code" requirements only apply to CPU0? Linux prepares the non-boot CPUs > for SMP, as you see here. > >> Also, if you are coming straight from reset, there is >> other setup probably missing. > > Like what? Errata work-arounds, performance bits, etc. I don't recall exactly which ones are per core vs. global. I assume this is an A9, but cores with virt extensions would also need to drop to non-secure mode. Also, your entry (and many of the custom entry points) would not work for big endian kernels. You may not care, but there's really no reason why it should not work. >> > + per_cpu(per_cpu_sw_state, cpu) = val; >> > + dmb(); >> > + sync_cache_w(SHIFT_PERCPU_PTR(&per_cpu_sw_state, per_cpu_offset(cpu))); >> > + dsb_sev(); >> >> Barriers need to be documented why they are needed. > > (NB: I see approximately 3 total existing cases where there is a comment > near a dsb()/dsb_sev().) > > This was actually adapted from the mcpm code, and Why? Blindly copying other implementations seems to be a common issue here... > (1) I think the dmb() is in the wrong place (it should be the first > thing in this function); > (2) the SEV is unnecessary (we don't use WFE; just a cache-invalidated > polling loop); and > (3) the DSB is also unnecessary, since sync_cache_w() handles its own > barriers. > > Plus, we should probably add some comments to describe what's going on > here. (Follow-up patch?) I'm not sure you need the state variable at all. You appear to be able to fully control power on and off of the cores, so you should not need any spin loops or synchronization. >> > +static int brcmstb_cpu_kill(u32 cpu) >> > +{ >> > + u32 tmp; >> > + >> > + pr_info("SMP: Powering down CPU%d...\n", cpu); >> > + >> > + while (per_cpu_sw_state_rd(cpu)) >> > + ; >> >> I don't think this is necessary. You don't need to synchronize die and >> kill. Look at other implementations that actually power down the core >> (vs. just wfi). > > Hmm, I'm pretty sure the synchronization is required for our B15 core. > If we yank the power before the core has properly quiesced, the whole > CPU complex will lock up. (Similar story for the power-up while loop you > complained about above.) That is true for every SMP system which is why it is done in the core code as Russell pointed out. >> > +static void brcmstb_secondary_init(unsigned int cpu) >> > +{ >> > + /* >> > + * Synchronise with the boot thread. >> > + */ >> > + spin_lock(&boot_lock); >> > + spin_unlock(&boot_lock); >> >> I suggest you read previous discussions on attempts to make this >> common before replying to Russell. > > Can you point me to this discussion? linux-arm-kernel is a big and noisy > world... Here's a recent example: http://www.spinics.net/lists/arm-kernel/msg318585.html > BTW, I'm not sure the boot_lock is actually necessary at all for us; it > was just borrowed from one of the other mach-*'s. The synchronization we > need is done in the while loops above. The blindly copying code without knowing what it does problem... Rob
Hello, 2014-08-01 7:33 GMT-07:00 Rob Herring <robherring2@gmail.com>: > On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris > <computersforpeace@gmail.com> wrote: >> Hi Rob, >> >> I appreciate your comments, but where were many of these 5 months ago on >> the first 7 revisions? :) > > Sorry, but that is the nature of upstreaming. But given some of the > issues, it is obvious the reviews were not sufficient. > >> On a practical note: v9 is already queued for 3.17. Should I send >> patches for the 3.17 cycle (or later) to fixup some of these issues? Or >> would you recommend pulling the patches out of Matt Porter's tree now, >> and reintroducing for 3.18? (I would be much happier with the first.) > > Things can always be un-queued. I guess that's Matt's and arm-soc's decision. Does that mean we should get all those patches un-queued, because that specific patch adding SMP support that needs to be reworked, or does that mean that if we drop this specific patch we are good with the remainder of the patch series? > >>> > +static const char *brcmstb_match[] __initconst = { >>> > + "brcm,bcm7445", >>> > + "brcm,brcmstb", >>> > + NULL >>> > +}; >>> > + >>> > +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)") >>> > + .dt_compat = brcmstb_match, >>> > +MACHINE_END >>> >>> Do you plan to add more here? Otherwise you don't need this file. >> >> Probably eventually, but not yet (there's out-of-tree stuff that hasn't >> been integrated); should we drop the file until it's needed? > > I would say yes if you re-spin the patches. > >>> > +ENTRY(brcmstb_secondary_startup) >>> > + /* >>> > + * Ensure CPU is in a sane state by disabling all IRQs and switching >>> > + * into SVC mode. >>> > + */ >>> > + setmode PSR_I_BIT | PSR_F_BIT | SVC_MODE, r0 >>> > + >>> > + bl v7_invalidate_l1 >>> >>> This should be in your boot code. That is part of the documented entry >>> requirements. >> >> By "documented" are you referring to the ARM TRM? Wouldn't any "boot >> code" requirements only apply to CPU0? Linux prepares the non-boot CPUs >> for SMP, as you see here. >> >>> Also, if you are coming straight from reset, there is >>> other setup probably missing. >> >> Like what? > > Errata work-arounds, performance bits, etc. I don't recall exactly > which ones are per core vs. global. I assume this is an A9, but cores > with virt extensions would also need to drop to non-secure mode. This is a Brahma B15 CPU, maybe we should have mentioned that again in this specific patch. The only ARM erratum we are affected (as of today) is 798181, and it requires a one-time initialization. > > Also, your entry (and many of the custom entry points) would not work > for big endian kernels. You may not care, but there's really no reason > why it should not work. I don't think we want to support big-endian kernels at all, although I can't see a good way to enforce that restriction. > >>> > + per_cpu(per_cpu_sw_state, cpu) = val; >>> > + dmb(); >>> > + sync_cache_w(SHIFT_PERCPU_PTR(&per_cpu_sw_state, per_cpu_offset(cpu))); >>> > + dsb_sev(); >>> >>> Barriers need to be documented why they are needed. >> >> (NB: I see approximately 3 total existing cases where there is a comment >> near a dsb()/dsb_sev().) >> >> This was actually adapted from the mcpm code, and > > Why? Blindly copying other implementations seems to be a common issue here... > >> (1) I think the dmb() is in the wrong place (it should be the first >> thing in this function); >> (2) the SEV is unnecessary (we don't use WFE; just a cache-invalidated >> polling loop); and >> (3) the DSB is also unnecessary, since sync_cache_w() handles its own >> barriers. >> >> Plus, we should probably add some comments to describe what's going on >> here. (Follow-up patch?) > > I'm not sure you need the state variable at all. You appear to be able > to fully control power on and off of the cores, so you should not need > any spin loops or synchronization. > >>> > +static int brcmstb_cpu_kill(u32 cpu) >>> > +{ >>> > + u32 tmp; >>> > + >>> > + pr_info("SMP: Powering down CPU%d...\n", cpu); >>> > + >>> > + while (per_cpu_sw_state_rd(cpu)) >>> > + ; >>> >>> I don't think this is necessary. You don't need to synchronize die and >>> kill. Look at other implementations that actually power down the core >>> (vs. just wfi). >> >> Hmm, I'm pretty sure the synchronization is required for our B15 core. >> If we yank the power before the core has properly quiesced, the whole >> CPU complex will lock up. (Similar story for the power-up while loop you >> complained about above.) > > That is true for every SMP system which is why it is done in the core > code as Russell pointed out. > >>> > +static void brcmstb_secondary_init(unsigned int cpu) >>> > +{ >>> > + /* >>> > + * Synchronise with the boot thread. >>> > + */ >>> > + spin_lock(&boot_lock); >>> > + spin_unlock(&boot_lock); >>> >>> I suggest you read previous discussions on attempts to make this >>> common before replying to Russell. >> >> Can you point me to this discussion? linux-arm-kernel is a big and noisy >> world... > > Here's a recent example: > > http://www.spinics.net/lists/arm-kernel/msg318585.html > >> BTW, I'm not sure the boot_lock is actually necessary at all for us; it >> was just borrowed from one of the other mach-*'s. The synchronization we >> need is done in the while loops above. > > The blindly copying code without knowing what it does problem...
On Fri, Aug 01, 2014 at 12:29:11PM -0700, Florian Fainelli wrote: > Hello, > > 2014-08-01 7:33 GMT-07:00 Rob Herring <robherring2@gmail.com>: > > On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris > > <computersforpeace@gmail.com> wrote: > >> Hi Rob, > >> > >> I appreciate your comments, but where were many of these 5 months ago on > >> the first 7 revisions? :) > > > > Sorry, but that is the nature of upstreaming. But given some of the > > issues, it is obvious the reviews were not sufficient. > > > >> On a practical note: v9 is already queued for 3.17. Should I send > >> patches for the 3.17 cycle (or later) to fixup some of these issues? Or > >> would you recommend pulling the patches out of Matt Porter's tree now, > >> and reintroducing for 3.18? (I would be much happier with the first.) > > > > Things can always be un-queued. I guess that's Matt's and arm-soc's decision. > > Does that mean we should get all those patches un-queued, because that > specific patch adding SMP support that needs to be reworked, or does > that mean that if we drop this specific patch we are good with the > remainder of the patch series? Well, keep in mind that there's no specific patch adding SMP support. The patch here contains *all* of the actual code that goes through mach-bcm. The rest will go through Russell. Given what was missed, if we drop just this patch, we're left with just the DT, Kconfig, and MAINTAINERS changes. It doesn't seem like there's time to fix the problems now. It might be better to drop the whole series from arm-soc since it won't be functional in 3.17 if we drop just this patch. I'd like to see what Arnd and Olof think. I think there's value in leaving all the DT bits for 3.17. -Matt
Here's some more comments on this. On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote: > +static void brcmstb_cpu_die(u32 cpu) > +{ > + v7_exit_coherency_flush(all); This is ultimately what causes my builds to break: /tmp/ccSPowmq.s:171: Error: selected processor does not support ARM mode `isb ' /tmp/ccSPowmq.s:177: Error: selected processor does not support ARM mode `isb ' /tmp/ccSPowmq.s:178: Error: selected processor does not support ARM mode `dsb ' make[2]: *** [arch/arm/mach-bcm/platsmp-brcmstb.o] Error 1 It seems that v7_exit_coherency_flush() can only be used with code which is ARMv7 only. > + /* Prevent all interrupts from reaching this CPU. */ > + arch_local_irq_disable(); Why do you think it is necessary to disable interrupts here? Where have they been re-enabled since this bit of generic code: void __ref cpu_die(void) { unsigned int cpu = smp_processor_id(); idle_task_exit(); local_irq_disable(); and why arch_local_irq_disable() at that? Even if interrupts were enabled prior to your call to arch_local_irq_disable(), what do you think would be the effect of receiving an interrupt after you've exited coherency? > + > + /* > + * Final full barrier to ensure everything before this instruction has > + * quiesced. > + */ > + isb(); > + dsb(); If the call to arch_local_irq_disable() is removed, and v7_exit_coherency_flush() is fixed, then this is not required, because v7_exit_coherency_flush() already does this at the very end. > + > + per_cpu_sw_state_wr(cpu, 0); > + > + /* Sit and wait to die */ > + wfi(); > + > + /* We should never get here... */ > + panic("Spurious interrupt on CPU %d received!\n", cpu); You really should /not/ be calling panic here, because that uses data shared with the CPUs which are still coherent. This is akin to doing DMA into bits of the kernel space without dealing with the cache coherency issues. Moreover, if you read the comments on v7_exit_coherency_flush() about ldrex/strex, which are two instructions spinlocks use, you'll see that ldrex/strex must not be executed, which means you can't call any function which uses spinlocks. That rules out printk() et.al. printascii is fine, but that's only available when the low level debug stuff is enabled.
On Fri, Aug 01, 2014 at 09:33:44AM -0500, Rob Herring wrote: > On Wed, Jul 30, 2014 at 9:23 PM, Brian Norris > <computersforpeace@gmail.com> wrote: > > Like what? > > Errata work-arounds, performance bits, etc. I don't recall exactly > which ones are per core vs. global. I assume this is an A9, but cores > with virt extensions would also need to drop to non-secure mode. The kernel's bringup just before enabling the CPU deals with that stuff provided your CPU is running in secure mode, but you are right - we /do/ want to encourage platforms to do the errata fixups before they call the kernel via any entry point.
On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote: > Hi Russell, > > Yes, I noticed this. What I meant is that smp_ops.cpu_die() and > smp_ops.cpu_kill() are not synchronized. ... > We're not relying on the L1 cache, though. Don't sync_cache_{r,w}() > ensure all reads/writes reach at least the L2? What exactly are you trying to achieve here? > How does that ensure that the CPU is down by the time the work is > scheduled? It seems like this would just defer the work long enough that > it *most likely* has quiesced, but I don't see how this gives us a > better guarantee. Or maybe I'm missing something. (If so, please do > enlighten!) Note that I said a delayed work queue. The dying CPU runs a predictable sequence once cpu_die() has been entered - interrupts at the GIC have been programmed to be routed to other CPUs, interrupts at the CPU are masked, so the CPU isn't going to be doing anything else except executing that code path. So, it's going to be a predictable number of CPU cycles. That allows you to arrange a delayed workqueue (or a timer) to fire after a period of time that you can guarantee that the dying CPU has reached that wfi(). Another point which raises itself in your patch is this: + /* Settle-time from Broadcom-internal DVT reference code */ + udelay(7); 7us looks very precise, but udelay() may not be that precise. What is the actual specification? Does it say "you must wait at least 7us"? udelay() _may_ return early, especially if it is using the CPU delay loop to perform the delay - I've explained why this happens previously, and why it isn't a bug. If you're using a timer-based delay for udelay() (which you should be using if you support cpufreq) then the delay should be more accurate, but it's still good practise to give a little leeway on the figure.
On Sat, Aug 02, 2014 at 09:19:24AM +0100, Russell King wrote: > Here's some more comments on this. > > On Mon, Jul 21, 2014 at 02:07:56PM -0700, Brian Norris wrote: > > +static void brcmstb_cpu_die(u32 cpu) > > +{ > > + v7_exit_coherency_flush(all); > > This is ultimately what causes my builds to break: > > /tmp/ccSPowmq.s:171: Error: selected processor does not support ARM mode `isb ' > /tmp/ccSPowmq.s:177: Error: selected processor does not support ARM mode `isb ' > /tmp/ccSPowmq.s:178: Error: selected processor does not support ARM mode `dsb ' > make[2]: *** [arch/arm/mach-bcm/platsmp-brcmstb.o] Error 1 > > It seems that v7_exit_coherency_flush() can only be used with code which > is ARMv7 only. Yes, I noticed this already, and I proposed a solution: http://article.gmane.org/gmane.linux.drivers.devicetree/84517 > > + /* Prevent all interrupts from reaching this CPU. */ > > + arch_local_irq_disable(); > > Why do you think it is necessary to disable interrupts here? Where > have they been re-enabled since this bit of generic code: > > void __ref cpu_die(void) > { > unsigned int cpu = smp_processor_id(); > > idle_task_exit(); > > local_irq_disable(); > > and why arch_local_irq_disable() at that? Even if interrupts were > enabled prior to your call to arch_local_irq_disable(), what do you > think would be the effect of receiving an interrupt after you've > exited coherency? This mistake was already noted. No need for the extra IRQ disable. (http://article.gmane.org/gmane.linux.drivers.devicetree/84516) > > + > > + /* > > + * Final full barrier to ensure everything before this instruction has > > + * quiesced. > > + */ > > + isb(); > > + dsb(); > > If the call to arch_local_irq_disable() is removed, and > v7_exit_coherency_flush() is fixed, then this is not required, because > v7_exit_coherency_flush() already does this at the very end. Right. Will drop. > > + > > + per_cpu_sw_state_wr(cpu, 0); > > + > > + /* Sit and wait to die */ > > + wfi(); > > + > > + /* We should never get here... */ > > + panic("Spurious interrupt on CPU %d received!\n", cpu); > > You really should /not/ be calling panic here, because that uses data > shared with the CPUs which are still coherent. This is akin to doing > DMA into bits of the kernel space without dealing with the cache > coherency issues. OK. > Moreover, if you read the comments on > v7_exit_coherency_flush() about ldrex/strex, which are two instructions > spinlocks use, you'll see that ldrex/strex must not be executed, which > means you can't call any function which uses spinlocks. That rules > out printk() et.al. printascii is fine, but that's only available when > the low level debug stuff is enabled. OK, so I'll drop the panic(). printascii doesn't look extremely useful, but I suppose we could use it for debugging. Seems like a while (1) loop might be a suitable replacement. If we get this far, we'll likely get locked up trying to power this CPU off anyway, so it'll be apparent that there was power-down failure. Thanks, Brian
Hi Russell, Picking up this thread again, as things are now set for dropping this patch and resubmitting SMP support for 3.18. On Sat, Aug 02, 2014 at 10:27:56AM +0100, Russell King wrote: > On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote: > > Yes, I noticed this. What I meant is that smp_ops.cpu_die() and > > smp_ops.cpu_kill() are not synchronized. > ... > > We're not relying on the L1 cache, though. Don't sync_cache_{r,w}() > > ensure all reads/writes reach at least the L2? > > What exactly are you trying to achieve here? Synchronization between v7_exit_coherency_flush() (on the dying CPU) and yanking the power (brcmstb_cpu_kill(), on the murderous CPU). The core completion-based synchronization is not sufficient, since it allows brcmstb_smp_ops.smp_kill and brcmstb_smp_ops.smp_die to race. Am I somehow not achieving what I intend here? > > How does that ensure that the CPU is down by the time the work is > > scheduled? It seems like this would just defer the work long enough that > > it *most likely* has quiesced, but I don't see how this gives us a > > better guarantee. Or maybe I'm missing something. (If so, please do > > enlighten!) > > Note that I said a delayed work queue. The dying CPU runs a predictable > sequence once cpu_die() has been entered - interrupts at the GIC have > been programmed to be routed to other CPUs, interrupts at the CPU are > masked, so the CPU isn't going to be doing anything else except executing > that code path. So, it's going to be a predictable number of CPU cycles. > > That allows you to arrange a delayed workqueue (or a timer) to fire > after a period of time that you can guarantee that the dying CPU has > reached that wfi(). OK, that sounds workable for the active hotplug case. But what about for the suspend case? CPUs are hot-unplugged during disable_nonboot_cpus(), and I don't see that this would guarantee the workqueue will complete before we enter suspend. > Another point which raises itself in your patch is this: > > + /* Settle-time from Broadcom-internal DVT reference code */ > + udelay(7); > > 7us looks very precise, but udelay() may not be that precise. What is > the actual specification? Does it say "you must wait at least 7us"? > > udelay() _may_ return early, especially if it is using the CPU delay > loop to perform the delay - I've explained why this happens previously, > and why it isn't a bug. > > If you're using a timer-based delay for udelay() (which you should be > using if you support cpufreq) then the delay should be more accurate, > but it's still good practise to give a little leeway on the figure. I'm looking into this specific delay. I'd bet it's just "wait at least 7us." I could probably factor in some leeway to be safe. Thanks, Brian
Hi Russell, On Wed, Aug 13, 2014 at 04:47:06PM -0700, Brian Norris wrote: > Picking up this thread again, as things are now set for dropping this > patch and resubmitting SMP support for 3.18. Any further comment on this? I'd like to submit v10 of this patch soon. As of now, my patch will still look essentially the same (with only minor changes for some of the review comments), since I'm convinced neither that my current per-cpu flag-based solution is unsound nor that a delayed work queue can solve all of the same problems. > On Sat, Aug 02, 2014 at 10:27:56AM +0100, Russell King wrote: > > On Thu, Jul 31, 2014 at 03:06:42PM -0700, Brian Norris wrote: > > > Yes, I noticed this. What I meant is that smp_ops.cpu_die() and > > > smp_ops.cpu_kill() are not synchronized. > > ... > > > We're not relying on the L1 cache, though. Don't sync_cache_{r,w}() > > > ensure all reads/writes reach at least the L2? > > > > What exactly are you trying to achieve here? > > Synchronization between v7_exit_coherency_flush() (on the dying CPU) and > yanking the power (brcmstb_cpu_kill(), on the murderous CPU). The core > completion-based synchronization is not sufficient, since it allows > brcmstb_smp_ops.smp_kill and brcmstb_smp_ops.smp_die to race. > > Am I somehow not achieving what I intend here? > > > > How does that ensure that the CPU is down by the time the work is > > > scheduled? It seems like this would just defer the work long enough that > > > it *most likely* has quiesced, but I don't see how this gives us a > > > better guarantee. Or maybe I'm missing something. (If so, please do > > > enlighten!) > > > > Note that I said a delayed work queue. The dying CPU runs a predictable > > sequence once cpu_die() has been entered - interrupts at the GIC have > > been programmed to be routed to other CPUs, interrupts at the CPU are > > masked, so the CPU isn't going to be doing anything else except executing > > that code path. So, it's going to be a predictable number of CPU cycles. > > > > That allows you to arrange a delayed workqueue (or a timer) to fire > > after a period of time that you can guarantee that the dying CPU has > > reached that wfi(). > > OK, that sounds workable for the active hotplug case. > > But what about for the suspend case? CPUs are hot-unplugged during > disable_nonboot_cpus(), and I don't see that this would guarantee the > workqueue will complete before we enter suspend. > > > Another point which raises itself in your patch is this: > > > > + /* Settle-time from Broadcom-internal DVT reference code */ > > + udelay(7); > > [...] > I'm looking into this specific delay. [...] BTW, I think this delay can be dropped. It was retained from an early non-production release of this CPU. Brian
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig index 534836497998..bf0df396a3cf 100644 --- a/arch/arm/configs/multi_v7_defconfig +++ b/arch/arm/configs/multi_v7_defconfig @@ -19,6 +19,7 @@ CONFIG_MACH_DOVE=y CONFIG_ARCH_BCM=y CONFIG_ARCH_BCM_MOBILE=y CONFIG_ARCH_BCM_5301X=y +CONFIG_ARCH_BRCMSTB=y CONFIG_ARCH_BERLIN=y CONFIG_MACH_BERLIN_BG2=y CONFIG_MACH_BERLIN_BG2CD=y diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig index 41c839167e87..0073633e7699 100644 --- a/arch/arm/mach-bcm/Kconfig +++ b/arch/arm/mach-bcm/Kconfig @@ -87,4 +87,18 @@ config ARCH_BCM_5301X different SoC or with the older BCM47XX and BCM53XX based network SoC using a MIPS CPU, they are supported by arch/mips/bcm47xx +config ARCH_BRCMSTB + bool "Broadcom BCM7XXX based boards" if ARCH_MULTI_V7 + depends on MMU + select ARM_GIC + select MIGHT_HAVE_PCI + select HAVE_SMP + select HAVE_ARM_ARCH_TIMER + help + Say Y if you intend to run the kernel on a Broadcom ARM-based STB + chipset. + + This enables support for Broadcom ARM-based set-top box chipsets, + including the 7445 family of chips. + endif diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile index 731292114975..f3665121729b 100644 --- a/arch/arm/mach-bcm/Makefile +++ b/arch/arm/mach-bcm/Makefile @@ -30,3 +30,8 @@ obj-$(CONFIG_ARCH_BCM2835) += board_bcm2835.o # BCM5301X obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o + +ifeq ($(CONFIG_ARCH_BRCMSTB),y) +obj-y += brcmstb.o +obj-$(CONFIG_SMP) += headsmp-brcmstb.o platsmp-brcmstb.o +endif diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c new file mode 100644 index 000000000000..60a5afa06ed7 --- /dev/null +++ b/arch/arm/mach-bcm/brcmstb.c @@ -0,0 +1,28 @@ +/* + * Copyright (C) 2013-2014 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/init.h> +#include <linux/of_platform.h> + +#include <asm/mach-types.h> +#include <asm/mach/arch.h> + +static const char *brcmstb_match[] __initconst = { + "brcm,bcm7445", + "brcm,brcmstb", + NULL +}; + +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)") + .dt_compat = brcmstb_match, +MACHINE_END diff --git a/arch/arm/mach-bcm/brcmstb.h b/arch/arm/mach-bcm/brcmstb.h new file mode 100644 index 000000000000..ec0c3d112b36 --- /dev/null +++ b/arch/arm/mach-bcm/brcmstb.h @@ -0,0 +1,19 @@ +/* + * Copyright (C) 2013-2014 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __BRCMSTB_H__ +#define __BRCMSTB_H__ + +void brcmstb_secondary_startup(void); + +#endif /* __BRCMSTB_H__ */ diff --git a/arch/arm/mach-bcm/headsmp-brcmstb.S b/arch/arm/mach-bcm/headsmp-brcmstb.S new file mode 100644 index 000000000000..199c1ea58248 --- /dev/null +++ b/arch/arm/mach-bcm/headsmp-brcmstb.S @@ -0,0 +1,33 @@ +/* + * SMP boot code for secondary CPUs + * Based on arch/arm/mach-tegra/headsmp.S + * + * Copyright (C) 2010 NVIDIA, Inc. + * Copyright (C) 2013-2014 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <asm/assembler.h> +#include <linux/linkage.h> +#include <linux/init.h> + + .section ".text.head", "ax" + +ENTRY(brcmstb_secondary_startup) + /* + * Ensure CPU is in a sane state by disabling all IRQs and switching + * into SVC mode. + */ + setmode PSR_I_BIT | PSR_F_BIT | SVC_MODE, r0 + + bl v7_invalidate_l1 + b secondary_startup +ENDPROC(brcmstb_secondary_startup) diff --git a/arch/arm/mach-bcm/platsmp-brcmstb.c b/arch/arm/mach-bcm/platsmp-brcmstb.c new file mode 100644 index 000000000000..af780e9c23a6 --- /dev/null +++ b/arch/arm/mach-bcm/platsmp-brcmstb.c @@ -0,0 +1,363 @@ +/* + * Broadcom STB CPU SMP and hotplug support for ARM + * + * Copyright (C) 2013-2014 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/delay.h> +#include <linux/errno.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <linux/printk.h> +#include <linux/regmap.h> +#include <linux/smp.h> +#include <linux/mfd/syscon.h> +#include <linux/spinlock.h> + +#include <asm/cacheflush.h> +#include <asm/cp15.h> +#include <asm/mach-types.h> +#include <asm/smp_plat.h> + +#include "brcmstb.h" + +enum { + ZONE_MAN_CLKEN_MASK = BIT(0), + ZONE_MAN_RESET_CNTL_MASK = BIT(1), + ZONE_MAN_MEM_PWR_MASK = BIT(4), + ZONE_RESERVED_1_MASK = BIT(5), + ZONE_MAN_ISO_CNTL_MASK = BIT(6), + ZONE_MANUAL_CONTROL_MASK = BIT(7), + ZONE_PWR_DN_REQ_MASK = BIT(9), + ZONE_PWR_UP_REQ_MASK = BIT(10), + ZONE_BLK_RST_ASSERT_MASK = BIT(12), + ZONE_PWR_OFF_STATE_MASK = BIT(25), + ZONE_PWR_ON_STATE_MASK = BIT(26), + ZONE_DPG_PWR_STATE_MASK = BIT(28), + ZONE_MEM_PWR_STATE_MASK = BIT(29), + ZONE_RESET_STATE_MASK = BIT(31), + CPU0_PWR_ZONE_CTRL_REG = 1, + CPU_RESET_CONFIG_REG = 2, +}; + +static void __iomem *cpubiuctrl_block; +static void __iomem *hif_cont_block; +static u32 cpu0_pwr_zone_ctrl_reg; +static u32 cpu_rst_cfg_reg; +static u32 hif_cont_reg; + +#ifdef CONFIG_HOTPLUG_CPU +static DEFINE_PER_CPU_ALIGNED(int, per_cpu_sw_state); + +static int per_cpu_sw_state_rd(u32 cpu) +{ + sync_cache_r(SHIFT_PERCPU_PTR(&per_cpu_sw_state, per_cpu_offset(cpu))); + return per_cpu(per_cpu_sw_state, cpu); +} + +static void per_cpu_sw_state_wr(u32 cpu, int val) +{ + per_cpu(per_cpu_sw_state, cpu) = val; + dmb(); + sync_cache_w(SHIFT_PERCPU_PTR(&per_cpu_sw_state, per_cpu_offset(cpu))); + dsb_sev(); +} +#else +static inline void per_cpu_sw_state_wr(u32 cpu, int val) { } +#endif + +static void __iomem *pwr_ctrl_get_base(u32 cpu) +{ + void __iomem *base = cpubiuctrl_block + cpu0_pwr_zone_ctrl_reg; + base += (cpu_logical_map(cpu) * 4); + return base; +} + +static u32 pwr_ctrl_rd(u32 cpu) +{ + void __iomem *base = pwr_ctrl_get_base(cpu); + return readl_relaxed(base); +} + +static void pwr_ctrl_wr(u32 cpu, u32 val) +{ + void __iomem *base = pwr_ctrl_get_base(cpu); + writel(val, base); +} + +static void cpu_rst_cfg_set(u32 cpu, int set) +{ + u32 val; + val = readl_relaxed(cpubiuctrl_block + cpu_rst_cfg_reg); + if (set) + val |= BIT(cpu_logical_map(cpu)); + else + val &= ~BIT(cpu_logical_map(cpu)); + writel_relaxed(val, cpubiuctrl_block + cpu_rst_cfg_reg); +} + +static void cpu_set_boot_addr(u32 cpu, unsigned long boot_addr) +{ + const int reg_ofs = cpu_logical_map(cpu) * 8; + writel_relaxed(0, hif_cont_block + hif_cont_reg + reg_ofs); + writel_relaxed(boot_addr, hif_cont_block + hif_cont_reg + 4 + reg_ofs); +} + +static void brcmstb_cpu_boot(u32 cpu) +{ + pr_info("SMP: Booting CPU%d...\n", cpu); + + /* + * set the reset vector to point to the secondary_startup + * routine + */ + cpu_set_boot_addr(cpu, virt_to_phys(brcmstb_secondary_startup)); + + /* unhalt the cpu */ + cpu_rst_cfg_set(cpu, 0); +} + +static void brcmstb_cpu_power_on(u32 cpu) +{ + /* + * The secondary cores power was cut, so we must go through + * power-on initialization. + */ + u32 tmp; + + pr_info("SMP: Powering up CPU%d...\n", cpu); + + /* Request zone power up */ + pwr_ctrl_wr(cpu, ZONE_PWR_UP_REQ_MASK); + + /* Wait for the power up FSM to complete */ + do { + tmp = pwr_ctrl_rd(cpu); + } while (!(tmp & ZONE_PWR_ON_STATE_MASK)); + + per_cpu_sw_state_wr(cpu, 1); +} + +static int brcmstb_cpu_get_power_state(u32 cpu) +{ + int tmp = pwr_ctrl_rd(cpu); + return (tmp & ZONE_RESET_STATE_MASK) ? 0 : 1; +} + +#ifdef CONFIG_HOTPLUG_CPU + +static void brcmstb_cpu_die(u32 cpu) +{ + v7_exit_coherency_flush(all); + + /* Prevent all interrupts from reaching this CPU. */ + arch_local_irq_disable(); + + /* + * Final full barrier to ensure everything before this instruction has + * quiesced. + */ + isb(); + dsb(); + + per_cpu_sw_state_wr(cpu, 0); + + /* Sit and wait to die */ + wfi(); + + /* We should never get here... */ + panic("Spurious interrupt on CPU %d received!\n", cpu); +} + +static int brcmstb_cpu_kill(u32 cpu) +{ + u32 tmp; + + pr_info("SMP: Powering down CPU%d...\n", cpu); + + while (per_cpu_sw_state_rd(cpu)) + ; + + /* Program zone reset */ + pwr_ctrl_wr(cpu, ZONE_RESET_STATE_MASK | ZONE_BLK_RST_ASSERT_MASK | + ZONE_PWR_DN_REQ_MASK); + + /* Verify zone reset */ + tmp = pwr_ctrl_rd(cpu); + if (!(tmp & ZONE_RESET_STATE_MASK)) + pr_err("%s: Zone reset bit for CPU %d not asserted!\n", + __func__, cpu); + + /* Wait for power down */ + do { + tmp = pwr_ctrl_rd(cpu); + } while (!(tmp & ZONE_PWR_OFF_STATE_MASK)); + + /* Settle-time from Broadcom-internal DVT reference code */ + udelay(7); + + /* Assert reset on the CPU */ + cpu_rst_cfg_set(cpu, 1); + + return 1; +} + +#endif /* CONFIG_HOTPLUG_CPU */ + +static int __init setup_hifcpubiuctrl_regs(struct device_node *np) +{ + int rc = 0; + char *name; + struct device_node *syscon_np = NULL; + + name = "syscon-cpu"; + + syscon_np = of_parse_phandle(np, name, 0); + if (!syscon_np) { + pr_err("can't find phandle %s\n", name); + rc = -EINVAL; + goto cleanup; + } + + cpubiuctrl_block = of_iomap(syscon_np, 0); + if (!cpubiuctrl_block) { + pr_err("iomap failed for cpubiuctrl_block\n"); + rc = -EINVAL; + goto cleanup; + } + + rc = of_property_read_u32_index(np, name, CPU0_PWR_ZONE_CTRL_REG, + &cpu0_pwr_zone_ctrl_reg); + if (rc) { + pr_err("failed to read 1st entry from %s property (%d)\n", name, + rc); + rc = -EINVAL; + goto cleanup; + } + + rc = of_property_read_u32_index(np, name, CPU_RESET_CONFIG_REG, + &cpu_rst_cfg_reg); + if (rc) { + pr_err("failed to read 2nd entry from %s property (%d)\n", name, + rc); + rc = -EINVAL; + goto cleanup; + } + +cleanup: + if (syscon_np) + of_node_put(syscon_np); + + return rc; +} + +static int __init setup_hifcont_regs(struct device_node *np) +{ + int rc = 0; + char *name; + struct device_node *syscon_np = NULL; + + name = "syscon-cont"; + + syscon_np = of_parse_phandle(np, name, 0); + if (!syscon_np) { + pr_err("can't find phandle %s\n", name); + rc = -EINVAL; + goto cleanup; + } + + hif_cont_block = of_iomap(syscon_np, 0); + if (!hif_cont_block) { + pr_err("iomap failed for hif_cont_block\n"); + rc = -EINVAL; + goto cleanup; + } + + /* offset is at top of hif_cont_block */ + hif_cont_reg = 0; + +cleanup: + if (syscon_np) + of_node_put(syscon_np); + + return rc; +} + +static void __init brcmstb_cpu_ctrl_setup(unsigned int max_cpus) +{ + int rc; + struct device_node *np; + char *name; + + name = "brcm,brcmstb-smpboot"; + np = of_find_compatible_node(NULL, NULL, name); + if (!np) { + pr_err("can't find compatible node %s\n", name); + return; + } + + rc = setup_hifcpubiuctrl_regs(np); + if (rc) + return; + + rc = setup_hifcont_regs(np); + if (rc) + return; +} + +static DEFINE_SPINLOCK(boot_lock); + +static void brcmstb_secondary_init(unsigned int cpu) +{ + /* + * Synchronise with the boot thread. + */ + spin_lock(&boot_lock); + spin_unlock(&boot_lock); +} + +static int brcmstb_boot_secondary(unsigned int cpu, struct task_struct *idle) +{ + /* + * set synchronisation state between this boot processor + * and the secondary one + */ + spin_lock(&boot_lock); + + /* Bring up power to the core if necessary */ + if (brcmstb_cpu_get_power_state(cpu) == 0) + brcmstb_cpu_power_on(cpu); + + brcmstb_cpu_boot(cpu); + + /* + * now the secondary core is starting up let it run its + * calibrations, then wait for it to finish + */ + spin_unlock(&boot_lock); + + return 0; +} + +static struct smp_operations brcmstb_smp_ops __initdata = { + .smp_prepare_cpus = brcmstb_cpu_ctrl_setup, + .smp_secondary_init = brcmstb_secondary_init, + .smp_boot_secondary = brcmstb_boot_secondary, +#ifdef CONFIG_HOTPLUG_CPU + .cpu_kill = brcmstb_cpu_kill, + .cpu_die = brcmstb_cpu_die, +#endif +}; + +CPU_METHOD_OF_DECLARE(brcmstb_smp, "brcm,brahma-b15", &brcmstb_smp_ops);