Re: [PATCH v5 1/8] ARM: brcmstb: add infrastructure for ARM-basedBroadcom STB SoCs

From: Mark Rutland
Date: Fri Jan 24 2014 - 05:15:00 EST

Next message: Ingo Molnar: "Re: [PATCH 02/11] x86: sysfb: remove sysfb when probing real hw"
Previous message: Mel Gorman: "Re: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging, v2"
Next in thread: Marc C: "Re: [PATCH v5 1/8] ARM: brcmstb: add infrastructure for ARM-basedBroadcom STB SoCs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <marc.ceeeee@xxxxxxxxx>
> Acked-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> ---
> 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 <linux/clk-provider.h>
> +#include <linux/console.h>
> +#include <linux/clocksource.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/jiffies.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/smp.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/mach-types.h>
> +#include <asm/mach/arch.h>
> +#include <asm/mach/map.h>
> +#include <asm/mach/time.h>
> +
> +#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 <linux/smp.h>
> +#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 <asm/assembler.h>
> +#include <linux/linkage.h>
> +#include <linux/init.h>
> +
> + .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 <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/jiffies.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/printk.h>
> +#include <linux/regmap.h>
> +#include <linux/smp.h>
> +#include <linux/mfd/syscon.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/mach-types.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(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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Ingo Molnar: "Re: [PATCH 02/11] x86: sysfb: remove sysfb when probing real hw"
Previous message: Mel Gorman: "Re: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging, v2"
Next in thread: Marc C: "Re: [PATCH v5 1/8] ARM: brcmstb: add infrastructure for ARM-basedBroadcom STB SoCs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]