Re: [PATCH V2 22/22] MIPS: Add multiplatform BMIPS target
From: Jonas Gorski
Date: Mon Nov 17 2014 - 09:45:21 EST
Hi,
On Sun, Nov 16, 2014 at 1:17 AM, Kevin Cernekee <cernekee@xxxxxxxxx> wrote:
> bmips_be_defconfig supports Linux running on the following CM and DSL
> SoCs:
>
> - BCM3384 (BMIPS5000) cable modem application processor, BE, SMP
> - BCM3384 (BMIPS4355) cable modem "spare CPU"*, BE
> - BCM6328 (BMIPS4355) ADSL chip, BE
Is BMIPS435*5* intentional? I would have assumed at least 6328 is also
MIPS4350 like BCM6368.
> - BCM6368 (BMIPS4350) ADSL chip, BE, SMP
>
> *experimental; most configurations will require changing CONFIG_PHYSICAL_START
>
> bmips_stb_defconfig supports Linux running on the (nominally LE) STB
> chipsets:
>
> - BCM7125 (BMIPS4380) set-top box chip, LE, SMP
> - BCM7346 (BMIPS5000) set-top box chip, LE, SMP
> - BCM7360 (BMIPS3300) set-top box chip, LE
> - BCM7420 (BMIPS5000) set-top box chip, LE, SMP
> - BCM7425 (BMIPS5000) set-top box chip, LE, SMP
>
> serial8250 and bcm63xx_uart do not currently coexist. If/when this is
> fixed, it will be also possible to boot the BE image on any supported STB
> board configured for BE. For now, each defconfig can only pick one UART
> driver, and the BE defconfig enables bcm63xx_uart.
>
> On these MIPS systems, endianness cannot be reconfigured at runtime. On
> STB it is sometimes offered as a board jumper or 0-ohm resistor, and
> sometimes hardwired to LE only. The CM and DSL systems always run BE.
>
> Device Tree is used to configure the following items:
>
> - UART, USB, GENET peripherals
> - IRQ controllers
> - Early console base address (bcm63xx_uart only)
> - SMP or UP mode
> - MIPS counter frequency
> - Memory size / regions
> - DMA remappings
> - Kernel command line
>
> The DT-enabled bootloader and build instructions for 3384 are posted at
> https://github.com/Broadcom/aeolus . The other chips use legacy non-DT
> bootloaders, and so the kernel employs autodetection heuristics to select
> a builtin DTB.
>
> Signed-off-by: Kevin Cernekee <cernekee@xxxxxxxxx>
This looks nice, and I think I agree with a new target instead of
trying to retrofit all the dtb/SoC support into bcm63xx.
Some individual comments here because I'm too lazy to search for the
introducing patches (more meant as general commentary than nitpicks).
(snip)
> diff --git a/Documentation/devicetree/bindings/mips/brcm/bmips.txt b/Documentation/devicetree/bindings/mips/brcm/bmips.txt
> new file mode 100644
> index 0000000..4a8cd8f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mips/brcm/bmips.txt
> @@ -0,0 +1,8 @@
> +* Broadcom MIPS (BMIPS) CPUs
> +
> +Required properties:
> +- compatible: "brcm,bmips3300", "brcm,bmips4350", "brcm,bmips4380"
> + "brcm,bmips5000"
> +
> +- mips-hpt-frequency: This is common to all CPUs in the system so it lives
> + under the "cpus" node.
Is it a good idea to hardcode this? Some SoC CPUs allow running with
different frequencies, which will directly affect this. Also I would
assume this would break once we add support for runtime clock changes
for BMIPS; at least on the DSL platform you can change the clock
between 1/4 (IIRC) and 1/1 for power saving.
> diff --git a/Documentation/devicetree/bindings/mips/brcm/soc.txt b/Documentation/devicetree/bindings/mips/brcm/soc.txt
> new file mode 100644
> index 0000000..1eedcb8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mips/brcm/soc.txt
> @@ -0,0 +1,21 @@
> +* Broadcom cable/DSL/settop platforms
> +
> +SoCs:
> +
> +Required properties:
> +- compatible: "brcm,bcm3384", "brcm,bcm33843"
> + "brcm,bcm3384-viper", "brcm,bcm33843-viper"
> + "brcm,bcm6328", "brcm,bcm6368",
> + "brcm,bcm7125", "brcm,bcm7346", "brcm,bcm7360",
> + "brcm,bcm7420", "brcm,bcm7425"
> +
> +Boards:
> +
> +Required properties:
> +- compatible: "brcm,bcm93384wvg", "brcm,bcm93384wvg-viper"
> + "brcm,bcm9ejtagprb", "brcm,bcm96368mvwg",
> + "brcm,bcm97125cbmb", "brcm,bcm97346dbsmb", "brcm,bcm97360svmb",
> + "brcm,bcm97420c", "brcm,bcm97425svmb"
Should the list of supported boards really be hardcoded here/in the
kernel? It doesn't match what the code does, as it (as far as I can
tell) accepts dtbs without any of the board compatible ids when passed
from bootloader.
> +
> +The experimental -viper variants are for running Linux on the 3384's
> +BMIPS4355 cable modem CPU instead of the BMIPS5000 application processor.
(snip)
> diff --git a/arch/mips/bmips/irq.c b/arch/mips/bmips/irq.c
> new file mode 100644
> index 0000000..14552e5
> --- /dev/null
> +++ b/arch/mips/bmips/irq.c
> @@ -0,0 +1,38 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * Copyright (C) 2014 Broadcom Corporation
> + * Author: Kevin Cernekee <cernekee@xxxxxxxxx>
> + */
> +
> +#include <linux/of.h>
> +#include <linux/irqchip.h>
> +
> +#include <asm/bmips.h>
> +#include <asm/irq.h>
> +#include <asm/irq_cpu.h>
> +#include <asm/time.h>
> +
> +unsigned int get_c0_compare_int(void)
> +{
> + return CP0_LEGACY_COMPARE_IRQ;
> +}
> +
> +void __init arch_init_irq(void)
> +{
> + struct device_node *dn;
> +
> + /* Only the STB (bcm7038) controller supports SMP IRQ affinity */
> + dn = of_find_compatible_node(NULL, NULL, "brcm,bcm7038-l1-intc");
> + if (dn)
> + of_node_put(dn);
> + else
> + bmips_tp1_irqs = 0;
> +
> + irqchip_init();
> +}
> +
> +OF_DECLARE_2(irqchip, mips_cpu_intc, "mti,cpu-interrupt-controller",
> + mips_cpu_irq_of_init);
> diff --git a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c
> new file mode 100644
> index 0000000..c34601d
> --- /dev/null
> +++ b/arch/mips/bmips/setup.c
> @@ -0,0 +1,249 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2014 Broadcom Corporation
> + * Author: Kevin Cernekee <cernekee@xxxxxxxxx>
> + */
> +
> +#include <linux/init.h>
> +#include <linux/bitops.h>
> +#include <linux/bootmem.h>
> +#include <linux/clk-provider.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/of_platform.h>
> +#include <linux/smp.h>
> +#include <asm/addrspace.h>
> +#include <asm/bmips.h>
> +#include <asm/bootinfo.h>
> +#include <asm/cpu-type.h>
> +#include <asm/mipsregs.h>
> +#include <asm/prom.h>
> +#include <asm/smp-ops.h>
> +#include <asm/time.h>
> +#include <asm/traps.h>
> +#include <asm/fw/cfe/cfe_api.h>
> +
> +#define CMT_LOCAL_TPID BIT(31)
> +#define RELO_NORMAL_VEC BIT(18)
> +
> +#define REG_DSL_CHIP_ID ((void __iomem *)CKSEG1ADDR(0x10000000))
> +#define REG_STB_CHIP_ID ((void __iomem *)CKSEG1ADDR(0x10404000))
> +
> +#define REG_BCM6328_OTP ((void __iomem *)CKSEG1ADDR(0x1000062c))
> +#define BCM6328_TP1_DISABLED BIT(9)
> +
> +static const unsigned long kbase = VMLINUX_LOAD_ADDRESS & 0xfff00000;
> +
> +struct bmips_board_list {
> + void *dtb;
> + u32 chip_id;
> + const char *boardname;
> + void (*quirk_fn)(void);
> +};
> +
> +extern char __dtb_bcm9ejtagprb_begin;
> +extern char __dtb_bcm96368mvwg_begin;
> +extern char __dtb_bcm97125cbmb_begin;
> +extern char __dtb_bcm97346dbsmb_begin;
> +extern char __dtb_bcm97360svmb_begin;
> +extern char __dtb_bcm97420c_begin;
> +extern char __dtb_bcm97425svmb_begin;
I think it would be a good idea to have the embedded dtbs optional,
especially if you already provide an interface for passing a dtb
pointer.
> +
> +static void kbase_setup(void)
> +{
> + __raw_writel(kbase | RELO_NORMAL_VEC,
> + BMIPS_GET_CBR() + BMIPS_RELO_VECTOR_CONTROL_1);
> + ebase = kbase;
> +}
> +
> +static void bcm3384_quirks(void)
> +{
> + /*
> + * Some experimental CM boxes are set up to let CM own the Viper TP0
> + * and let Linux own TP1. This requires moving the kernel
> + * load address to a non-conflicting region (e.g. via
> + * CONFIG_PHYSICAL_START) and supplying an alternate DTB.
> + * If we detect this condition, we need to move the MIPS exception
> + * vectors up to an area that we own.
> + *
> + * This is distinct from the OTHER special case mentioned in
> + * smp-bmips.c (boot on TP1, but enable SMP, then TP0 becomes our
> + * logical CPU#1). For the Viper TP1 case, SMP is off limits.
> + *
> + * Also note that many BMIPS435x CPUs do not have a
> + * BMIPS_RELO_VECTOR_CONTROL_1 register, so it isn't safe to just
> + * write VMLINUX_LOAD_ADDRESS into that register on every SoC.
> + */
> + if (current_cpu_type() == CPU_BMIPS4350 &&
> + kbase != CKSEG0 &&
> + read_c0_brcm_cmt_local() & CMT_LOCAL_TPID) {
> + board_ebase_setup = &kbase_setup;
> + bmips_smp_enabled = 0;
> + }
> +}
> +
> +static void bcm6328_quirks(void)
> +{
> + /* Check OTP to see if CPU1 is enabled (it usually isn't) */
> + if (__raw_readl(REG_BCM6328_OTP) & BCM6328_TP1_DISABLED)
> + bmips_smp_enabled = 0;
> +}
> +
> +static void bcm6368_quirks(void)
> +{
> + /*
> + * The bootloader has set up the CPU1 reset vector at
> + * 0xa000_0200.
> + * This conflicts with the special interrupt vector (IV).
> + * The bootloader has also set up CPU1 to respond to the wrong
> + * IPI interrupt.
> + * Here we will start up CPU1 in the background and ask it to
> + * reconfigure itself then go back to sleep.
> + */
> + memcpy((void *)0xa0000200, &bmips_smp_movevec, 0x20);
> + __sync();
> + set_c0_cause(C_SW0);
> + cpumask_set_cpu(1, &bmips_booted_mask);
> +}
> +
> +static const struct bmips_board_list bmips_board_list[] = {
> + { &__dtb_bcm9ejtagprb_begin, 0x6328, NULL, &bcm6328_quirks },
> + { &__dtb_bcm96368mvwg_begin, 0x6368, NULL, &bcm6368_quirks },
> + { &__dtb_bcm97125cbmb_begin, 0x7125, "BCM97125C0" },
> + { &__dtb_bcm97346dbsmb_begin, 0x7346, "BCM97346DBSMB" },
> + { &__dtb_bcm97360svmb_begin, 0x7360, "BCM97360SVMB" },
> + { &__dtb_bcm97420c_begin, 0x7420, "BCM97420C_DDR3" },
> + { &__dtb_bcm97425svmb_begin, 0x7425, "BCM97425SVMB" },
> + { },
> +};
> +
> +void __init prom_init(void)
> +{
> + register_bmips_smp_ops();
> +}
> +
> +void __init prom_free_prom_memory(void)
> +{
> +}
> +
> +const char *get_system_type(void)
> +{
> + return "BMIPS multiplatform kernel";
> +}
> +
> +void __init plat_time_init(void)
> +{
> + struct device_node *np;
> + u32 freq;
> +
> + np = of_find_node_by_name(NULL, "cpus");
> + if (!np)
> + panic("missing 'cpus' DT node");
> + if (of_property_read_u32(np, "mips-hpt-frequency", &freq) < 0)
> + panic("missing 'mips-hpt-frequency' property");
> + of_node_put(np);
> +
> + mips_hpt_frequency = freq;
> +}
> +
> +static void __init *find_dtb(void)
> +{
> + u32 chip_id;
> + char boardname[64] = "";
> + const struct bmips_board_list *b;
> +
> + /* Intended to somewhat resemble ARM; see Documentation/arm/Booting */
> + if (fw_arg0 == 0 && fw_arg1 == 0xffffffff)
> + return phys_to_virt(fw_arg2);
I know a bit late, but how about using the OF_DT_MAGIC (0xd00dfeed)
for indicating that there is a device tree in arg2?
> +
> + if (fw_arg1 != 0 || fw_arg3 != CFE_EPTSEAL ||
> + (fw_arg2 & 0xf0000000) != CKSEG0)
> + panic("cannot identify chip");
> +
> + /*
> + * Unfortunately the CFE API doesn't seem to provide chip
> + * identification, but we can check the entry point to see whether
> + * the current platform is a DSL chip or STB chip. On STB,
> + * CAE_STKSIZE = _regidx(13) = 13*8 = 104, so the first instruction is:
> + * 0: 23bdff98 addi sp,sp,-104
> + */
> + if (__raw_readl((void *)fw_arg2) == 0x23bdff98) {
> + chip_id = __raw_readl(REG_STB_CHIP_ID);
> + cfe_init(fw_arg0, fw_arg2);
> + cfe_getenv("CFE_BOARDNAME", boardname, sizeof(boardname));
> + } else {
> + /*
> + * This works on most modern chips, but will break on older
> + * ones like 6358
> + */
> + chip_id = __raw_readl(REG_DSL_CHIP_ID);
Unfortunately I don't know any good way to discriminate between the
"old" and the "new" chips except by looking a the PRID and REV
(BMIPS3300 <= 3.2 || BMIPS4350 <= 1.0 => 0xfff00000, BMIPS3300 >= 3.3
|| BMIPS4350 >= 3.0 => 0xb0000000).
> + }
> +
> + /* 4-digit parts use bits [31:16]; 5-digit parts use [27:8] */
This might be true for the STB chips, but the DSL 5-digit parts use
[31:12]. And to add insult to insury, some 4-digit parts use [15:12]
for the chip variant, so you can't just check [15:12] for 0 or != 0
either (I would assume 63381 and 6328 (variant 63281) will have both 1
at [15:12].
> + if (chip_id & 0xf0000000)
> + chip_id >>= 16;
> + else
> + chip_id >>= 8;
> +
> + for (b = bmips_board_list; b->dtb; b++) {
> + if (b->chip_id != chip_id)
> + continue;
> + if (b->boardname && strcmp(b->boardname, boardname))
> + continue;
> + if (b->quirk_fn)
> + b->quirk_fn();
Hmm, maybe move running the quirk out of here and into
plat_mem_setup()? Currently e.g. a BCM6328 with a bootloader passed
dtb won't have its quirk run.
> + return b->dtb;
> + }
> +
> + panic("no dtb found for current board");
> +}
> +
> +void __init plat_mem_setup(void)
> +{
> + set_io_port_base(0);
> + ioport_resource.start = 0;
> + ioport_resource.end = ~0;
> +
> + __dt_setup_arch(find_dtb());
> + strlcpy(arcs_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> +
> + /* BCM3384 DTB comes from the bootloader, not from bmips_board_list */
> + if (of_flat_dt_is_compatible(of_get_flat_dt_root(),
> + "brcm,bcm3384-viper")) {
> + bcm3384_quirks();
> + }
> +}
> +
> +void __init device_tree_init(void)
> +{
> + struct device_node *np;
> +
> + unflatten_and_copy_device_tree();
> +
> + /* Disable SMP boot unless both CPUs are listed in DT and !disabled */
> + np = of_find_node_by_name(NULL, "cpus");
> + if (np && of_get_available_child_count(np) <= 1)
> + bmips_smp_enabled = 0;
> + of_node_put(np);
> +}
> +
> +int __init plat_of_setup(void)
> +{
> + return __dt_register_buses("brcm,bmips", "simple-bus");
Huh, "brcm,bmips" does not appear anywhere else. How does this work?
Should this be a required compatible?
(OT: "buses" irks me because in my native tongue the plural uses "ss",
but I accept that using one "s" is also a valid spelling in english
:P)
> +}
> +
> +arch_initcall(plat_of_setup);
> +
> +static int __init plat_dev_init(void)
> +{
> + of_clk_init(NULL);
> + return 0;
> +}
> +
> +device_initcall(plat_dev_init);
> diff --git a/arch/mips/boot/dts/Makefile b/arch/mips/boot/dts/Makefile
> index ca9c90e..ffae96b 100644
> --- a/arch/mips/boot/dts/Makefile
> +++ b/arch/mips/boot/dts/Makefile
> @@ -1,3 +1,12 @@
> +dtb-$(CONFIG_BMIPS_MULTIPLATFORM) += bcm93384wvg.dtb \
Minor stilistic nitpick: I would use
dtb-$(CONFIG_BMIPS_MULTIPLATFORM) += \
bcm93384wvg.dtb \
so that adding a board before bcm963384wvg would only require an
insert, not also a modification.
> + bcm93384wvg_viper.dtb \
> + bcm96368mvwg.dtb \
> + bcm9ejtagprb.dtb \
> + bcm97125cbmb.dtb \
> + bcm97346dbsmb.dtb \
> + bcm97360svmb.dtb \
> + bcm97420c.dtb \
> + bcm97425svmb.dtb
> dtb-$(CONFIG_CAVIUM_OCTEON_SOC) += octeon_3xxx.dtb octeon_68xx.dtb
> dtb-$(CONFIG_DT_EASY50712) += easy50712.dtb
> dtb-$(CONFIG_DT_XLP_EVP) += xlp_evp.dtb
> diff --git a/arch/mips/boot/dts/bcm3384_common.dtsi b/arch/mips/boot/dts/bcm3384_common.dtsi
> new file mode 100644
> index 0000000..448cb5b
> --- /dev/null
> +++ b/arch/mips/boot/dts/bcm3384_common.dtsi
> @@ -0,0 +1,44 @@
> +/ {
> + clocks {
> + #address-cells = <1>;
This does not really make sense, as there is no address used at all
for the periph_clk.
> + #size-cells = <0>;
> +
> + periph_clk: periph_clk@0 {
Same for the @0 - there is no appropriate reg = <0>, so using an
address here does not make sense.
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <54000000>;
> + };
> + };
> +
> + aliases {
> + uart0 = &uart0;
> + };
> +
> + uart0: serial@14e00520 {
> + compatible = "brcm,bcm6345-uart";
> + reg = <0x14e00520 0x18>;
> + interrupt-parent = <&periph_intc>;
> + interrupts = <2>;
> + clocks = <&periph_clk>;
> + status = "disabled";
> + };
> +
> + ehci0: usb@15400300 {
> + compatible = "brcm,bcm3384-ehci", "generic-ehci";
> + reg = <0x15400300 0x100>;
> + big-endian;
> + interrupt-parent = <&periph_intc>;
> + interrupts = <41>;
> + status = "disabled";
> + };
> +
> + ohci0: usb@15400400 {
> + compatible = "brcm,bcm3384-ohci", "generic-ohci";
> + reg = <0x15400400 0x100>;
> + big-endian;
> + no-big-frame-no;
> + interrupt-parent = <&periph_intc>;
> + interrupts = <40>;
> + status = "disabled";
> + };
> +};
(snip)
> diff --git a/arch/mips/boot/dts/bcm6328.dtsi b/arch/mips/boot/dts/bcm6328.dtsi
> new file mode 100644
> index 0000000..a7e397f
> --- /dev/null
> +++ b/arch/mips/boot/dts/bcm6328.dtsi
> @@ -0,0 +1,63 @@
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "brcm,bcm6328";
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mips-hpt-frequency = <160000000>;
> +
> + cpu@0 {
> + compatible = "brcm,bmips4350";
> + device_type = "cpu";
> + reg = <0>;
> + };
Since there are SMP-enabled variants, maybe it should have its second
thread documented here (but defaulting to "disabled")?
> + };
> +
> + clocks {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + periph_clk: periph_clk@0 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <50000000>;
> + };
> + };
> +
> + aliases {
> + uart0 = &uart0;
> + };
> +
> + cpu_intc: cpu_intc@0 {
It does not have an address, so it should not have @0 in the node name I think.
> + #address-cells = <0>;
> + compatible = "mti,cpu-interrupt-controller";
> +
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + };
> +
> + periph_intc: periph_intc@10000024 {
> + compatible = "brcm,bcm7120-l2-intc";
> + reg = <0x10000024 0x4 0x1000002c 0x4
> + 0x10000020 0x4 0x10000028 0x4>;
The "lowest" register address you use is 0x10000020, so the name
should arguably be periph_intc@10000020, not periph_intc@10000024. I
guess the second cpu block (10000030 - 1000003c) wired to hw irq 3 can
be added later.
This will easily translate to a lot of io(re)map calls in case of
63268/6318 when describing both cpu blocks ( a total of 16 "reg"s).
Also I wonder how you properly describe the intc of 63381, where it
has separate mask registers, but a shared status register.
> +
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + interrupt-parent = <&cpu_intc>;
> + interrupts = <2>;
> + brcm,int-map-mask = <0xffffffff 0xffffffff>;
> + };
> +
> + uart0: serial@10000100 {
> + compatible = "brcm,bcm6345-uart";
> + reg = <0x10000100 0x18>;
> + interrupt-parent = <&periph_intc>;
> + interrupts = <28>;
> + clocks = <&periph_clk>;
> + status = "disabled";
> + };
> +};
(snip)
Jonas
--
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/