Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753122AbaAXV0O (ORCPT ); Fri, 24 Jan 2014 16:26:14 -0500 Received: from mail-pd0-f177.google.com ([209.85.192.177]:51184 "EHLO mail-pd0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752290AbaAXV0K (ORCPT ); Fri, 24 Jan 2014 16:26:10 -0500 Message-ID: <52E2DA6F.7050107@gmail.com> Date: Fri, 24 Jan 2014 13:26:07 -0800 From: Marc C User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Mark Rutland CC: Christian Daudt , Arnd Bergmann , "devicetree@vger.kernel.org" , Florian Fainelli , Russell King , "linux-kernel@vger.kernel.org" , Matt Porter , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH v5 1/8] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs References: <1390361452-3124-1-git-send-email-marc.ceeeee@gmail.com> <1390361452-3124-2-git-send-email-marc.ceeeee@gmail.com> <20140124101407.GL15586@e106331-lin.cambridge.arm.com> In-Reply-To: <20140124101407.GL15586@e106331-lin.cambridge.arm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Mark, >> +static void __init brcmstb_init_early(void) >> +{ >> + add_preferred_console("ttyS", 0, "115200"); >> +} > > Is this really required? I think I can drop this. It was a holdover from our older kernels. >> + /* >> + * set the reset vector to point to the secondary_startup >> + * routine >> + */ >> + cpu_set_boot_addr(cpu, virt_to_phys(brcmstb_secondary_startup)); >> + >> + flush_cache_all(); > > Why? What does the new CPU need before its caches are coherent and up? Absolutely nothing! I should be able to drop this as well. Regarding the CPU power-down sequence, I'll review it and make sure it follows the "Processor power domain" sequence in the A15 TRM. For any deviations, I'll double-check with our H/W designers to ensure there aren't any magic requirements unaccounted for. Thank you for taking a deep-dive into the code! I'll make the appropriate modifications per your suggestions. Regards, Marc C On 01/24/2014 02:14 AM, Mark Rutland wrote: > On Wed, Jan 22, 2014 at 03:30:45AM +0000, Marc Carino wrote: >> 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 >> Acked-by: Florian Fainelli >> --- >> arch/arm/configs/multi_v7_defconfig | 1 + >> arch/arm/mach-bcm/Kconfig | 14 ++ >> arch/arm/mach-bcm/Makefile | 4 + >> arch/arm/mach-bcm/brcmstb.c | 110 ++++++++++++ >> arch/arm/mach-bcm/brcmstb.h | 38 ++++ >> arch/arm/mach-bcm/headsmp-brcmstb.S | 34 ++++ >> arch/arm/mach-bcm/hotplug-brcmstb.c | 334 +++++++++++++++++++++++++++++++++++ >> 7 files changed, 535 insertions(+), 0 deletions(-) >> 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/hotplug-brcmstb.c >> >> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig >> index c1df4e9..7028d11 100644 >> --- a/arch/arm/configs/multi_v7_defconfig >> +++ b/arch/arm/configs/multi_v7_defconfig >> @@ -7,6 +7,7 @@ CONFIG_MACH_ARMADA_370=y >> CONFIG_MACH_ARMADA_XP=y >> CONFIG_ARCH_BCM=y >> CONFIG_ARCH_BCM_MOBILE=y >> +CONFIG_ARCH_BRCMSTB=y >> CONFIG_GPIO_PCA953X=y >> CONFIG_ARCH_HIGHBANK=y >> CONFIG_ARCH_KEYSTONE=y >> diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig >> index 9fe6d88..2c1ae83 100644 >> --- a/arch/arm/mach-bcm/Kconfig >> +++ b/arch/arm/mach-bcm/Kconfig >> @@ -31,6 +31,20 @@ config ARCH_BCM_MOBILE >> BCM11130, BCM11140, BCM11351, BCM28145 and >> BCM28155 variants. >> >> +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. >> + >> endmenu >> >> endif >> diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile >> index c2ccd5a..b744a12 100644 >> --- a/arch/arm/mach-bcm/Makefile >> +++ b/arch/arm/mach-bcm/Makefile >> @@ -13,3 +13,7 @@ >> obj-$(CONFIG_ARCH_BCM_MOBILE) := board_bcm281xx.o bcm_kona_smc.o bcm_kona_smc_asm.o kona.o >> plus_sec := $(call as-instr,.arch_extension sec,+sec) >> AFLAGS_bcm_kona_smc_asm.o :=-Wa,-march=armv7-a$(plus_sec) >> + >> +obj-$(CONFIG_ARCH_BRCMSTB) := brcmstb.o >> +obj-$(CONFIG_SMP) += headsmp-brcmstb.o >> +obj-$(CONFIG_HOTPLUG_CPU) += hotplug-brcmstb.o >> diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c >> new file mode 100644 >> index 0000000..7a6093d >> --- /dev/null >> +++ b/arch/arm/mach-bcm/brcmstb.c >> @@ -0,0 +1,110 @@ >> +/* >> + * Copyright (C) 2013 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "brcmstb.h" >> + >> +/*********************************************************************** >> + * STB CPU (main application processor) >> + ***********************************************************************/ >> + >> +static const char *brcmstb_match[] __initconst = { >> + "brcm,bcm7445", >> + "brcm,brcmstb", >> + NULL >> +}; >> + >> +static void __init brcmstb_init_early(void) >> +{ >> + add_preferred_console("ttyS", 0, "115200"); >> +} > > Is this really required? > >> + >> +/*********************************************************************** >> + * SMP boot >> + ***********************************************************************/ >> + >> +#ifdef CONFIG_SMP >> +static DEFINE_SPINLOCK(boot_lock); >> + >> +static void __cpuinit brcmstb_secondary_init(unsigned int cpu) >> +{ >> + /* >> + * Synchronise with the boot thread. >> + */ >> + spin_lock(&boot_lock); >> + spin_unlock(&boot_lock); >> +} >> + >> +static int __cpuinit 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; >> +} >> + >> +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 >> +}; >> +#endif >> + >> +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)") >> + .dt_compat = brcmstb_match, >> +#ifdef CONFIG_SMP >> + .smp = smp_ops(brcmstb_smp_ops), >> +#endif >> + .init_early = brcmstb_init_early, >> +MACHINE_END >> diff --git a/arch/arm/mach-bcm/brcmstb.h b/arch/arm/mach-bcm/brcmstb.h >> new file mode 100644 >> index 0000000..e49bde6 >> --- /dev/null >> +++ b/arch/arm/mach-bcm/brcmstb.h >> @@ -0,0 +1,38 @@ >> +/* >> + * Copyright (C) 2013 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__ >> + >> +#if !defined(__ASSEMBLY__) >> +#include >> +#endif >> + >> +#if !defined(__ASSEMBLY__) >> +extern void brcmstb_secondary_startup(void); >> +extern void brcmstb_cpu_boot(unsigned int cpu); >> +extern void brcmstb_cpu_power_on(unsigned int cpu); >> +extern int brcmstb_cpu_get_power_state(unsigned int cpu); >> +extern struct smp_operations brcmstb_smp_ops; >> +#if defined(CONFIG_HOTPLUG_CPU) >> +extern void brcmstb_cpu_die(unsigned int cpu); >> +extern int brcmstb_cpu_kill(unsigned int cpu); >> +void __init brcmstb_cpu_ctrl_setup(unsigned int max_cpus); >> +#else >> +static inline void brcmstb_cpu_die(unsigned int cpu) {} >> +static inline int brcmstb_cpu_kill(unsigned int cpu) {} >> +static inline void __init brcmstb_cpu_ctrl_setup(unsigned int max_cpus) {} >> +#endif >> +#endif >> + >> +#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 0000000..57ec438 >> --- /dev/null >> +++ b/arch/arm/mach-bcm/headsmp-brcmstb.S >> @@ -0,0 +1,34 @@ >> +/* >> + * SMP boot code for secondary CPUs >> + * Based on arch/arm/mach-tegra/headsmp.S >> + * >> + * Copyright (C) 2010 NVIDIA, Inc. >> + * Copyright (C) 2013 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 >> +#include >> +#include >> + >> + .section ".text.head", "ax" >> + __CPUINIT > > __CPUINIT is either going or gone by now. This should disappear. > >> + >> +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/hotplug-brcmstb.c b/arch/arm/mach-bcm/hotplug-brcmstb.c >> new file mode 100644 >> index 0000000..ff4a732 >> --- /dev/null >> +++ b/arch/arm/mach-bcm/hotplug-brcmstb.c >> @@ -0,0 +1,334 @@ >> +/* >> + * Broadcom STB CPU hotplug support for ARM >> + * >> + * Copyright (C) 2013 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> + >> +#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(10), >> + ZONE_PWR_OFF_STATE_MASK = BIT(26), >> + 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), >> +}; >> + >> +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; >> +DEFINE_PER_CPU(int, per_cpu_sw_state); >> + >> +static void __iomem *pwr_ctrl_get_base(unsigned int cpu) >> +{ >> + void __iomem *base = cpubiuctrl_block + cpu0_pwr_zone_ctrl_reg; >> + base += (cpu * 4); > > CPU isn't guaranteed to be the physical CPU ID (MPIDR.Aff*). While it > almost certainly will be, we can't guarantee it in the face of a kexec, > for example. > > You can use cpu_logical_map(cpu) to get the physical ID. > >> + return base; >> +} >> + >> +static u32 pwr_ctrl_rd(unsigned int cpu) >> +{ >> + void __iomem *base = pwr_ctrl_get_base(cpu); >> + return readl_relaxed(base); >> +} >> + >> +static void pwr_ctrl_wr(unsigned int cpu, u32 val) >> +{ >> + void __iomem *base = pwr_ctrl_get_base(cpu); >> + writel(val, base); >> +} >> + >> +static void cpu_rst_cfg_set(int cpu, int set) >> +{ >> + u32 val; >> + val = readl_relaxed(cpubiuctrl_block + cpu_rst_cfg_reg); >> + if (set) >> + val |= BIT(cpu); >> + else >> + val &= ~BIT(cpu); > > Likewise here. > >> + writel_relaxed(val, cpubiuctrl_block + cpu_rst_cfg_reg); >> +} >> + >> +static void cpu_set_boot_addr(int cpu, unsigned long boot_addr) >> +{ >> + const int reg_ofs = cpu * 8; > > And here. > >> + writel_relaxed(0, hif_cont_block + hif_cont_reg + reg_ofs); >> + writel_relaxed(boot_addr, hif_cont_block + hif_cont_reg + 4 + reg_ofs); >> +} >> + >> +void brcmstb_cpu_boot(unsigned int 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)); >> + >> + flush_cache_all(); > > Why? What does the new CPU need before its caches are coherent and up? > >> + >> + /* unhalt the cpu */ >> + cpu_rst_cfg_set(cpu, 0); >> +} >> + >> +void brcmstb_cpu_power_on(unsigned int 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(per_cpu_sw_state, cpu) = 1; >> +} >> + >> +int brcmstb_cpu_get_power_state(unsigned int cpu) >> +{ >> + int tmp = pwr_ctrl_rd(cpu); >> + return (tmp & ZONE_RESET_STATE_MASK) ? 0 : 1; >> +} >> + >> +void __ref brcmstb_cpu_die(unsigned int cpu) >> +{ >> + /* Derived from misc_bpcm_arm.c */ >> + >> + /* Clear SCTLR.C bit */ >> + __asm__( >> + "mrc p15, 0, r0, c1, c0, 0\n" >> + "bic r0, r0, #(1 << 2)\n" >> + "mcr p15, 0, r0, c1, c0, 0\n" >> + : /* no output */ >> + : /* no input */ >> + : "r0" /* clobber r0 */ >> + ); > > This is odd. Why not allow GCC to allocate the register? > >> + >> + /* >> + * Instruction barrier to ensure cache is really disabled before >> + * cleaning/invalidating the caches >> + */ >> + isb(); > > I think you could use: > > set_cr(get_cr() & ~CR_C)) > > Which would do all of the above (including the isb), and will get GCC to > allocate the register. > >> + >> + flush_cache_all(); >> + >> + /* Invalidate all instruction caches to PoU (ICIALLU) */ >> + /* Data sync. barrier to ensure caches have emptied out */ >> + __asm__("mcr p15, 0, r0, c7, c5, 0\n" : : : "r0"); >> + dsb(); > > Why do you need to invalidate the I-cache? > >> + >> + /* >> + * Clear ACTLR.SMP bit to prevent broadcast TLB messages from reaching >> + * this core >> + */ >> + __asm__( >> + "mrc p15, 0, r0, c1, c0, 1\n" >> + "bic r0, r0, #(1 << 6)\n" >> + "mcr p15, 0, r0, c1, c0, 1\n" >> + : /* no output */ >> + : /* no input */ >> + : "r0" /* clobber r0 */ >> + ); > > Surely you can use an output operand to get GCC to allocate the register > for you? > >> + >> + /* Disable all IRQs for this CPU */ >> + arch_local_irq_disable(); >> + >> + per_cpu(per_cpu_sw_state, cpu) = 0; > > Your caches are off at this point, so this could be going straight to > memory. Yet readers of this value aren't cleaning their caches before > reading this, so they could hit a stale cached copy. > >> + >> + /* >> + * Final full barrier to ensure everything before this instruction has >> + * quiesced. >> + */ >> + isb(); >> + dsb(); >> + >> + /* Sit and wait to die */ >> + wfi(); >> + >> + /* We should never get here... */ >> + nop(); > > Why the nop first? > >> + panic("Spurious interrupt on CPU %d received!\n", cpu); >> +} >> + >> +int brcmstb_cpu_kill(unsigned int cpu) >> +{ >> + u32 tmp; >> + >> + pr_info("SMP: Powering down CPU%d...\n", cpu); >> + >> + while (per_cpu(per_cpu_sw_state, cpu)) >> + ; > > As this was written to with caches disabled, the cached copy of the > value (which this is reading) could be stale. Surely you need to > clean+invalidate the line for this value each time you read it to give > it a chance to update? > >> + >> + /* 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; >> +} >> + >> +static int __init setup_hifcpubiuctrl_regs(struct device_node *np) >> +{ >> + int rc = 0; >> + char *name; >> + int index; >> + 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; >> + } >> + >> + index = 1; >> + rc = of_property_read_u32_index(np, name, index, >> + &cpu0_pwr_zone_ctrl_reg); > > The index variable seems rather pointless. Why not just use the value > in-place? > >> + if (rc) { >> + pr_err("failed to read %d from %s property (%d)\n", index, name, >> + rc); > > It might be better to state _what_ you're looking for (what does the > value represent?). > >> + rc = -EINVAL; >> + goto cleanup; >> + } >> + >> + index = 2; >> + rc = of_property_read_u32_index(np, name, index, &cpu_rst_cfg_reg); > > Likewise for all of the above. > > Thanks, > Mark. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/