[PATCH 1/8] ARM: support for Moschip MCS814x SoCs
Arnd Bergmann
arnd at arndb.de
Tue Jul 17 01:54:04 EST 2012
On Sunday 15 July 2012, Florian Fainelli wrote:
> diff --git a/arch/arm/mach-mcs814x/Kconfig b/arch/arm/mach-mcs814x/Kconfig
> new file mode 100644
> index 0000000..c89422f
> --- /dev/null
> +++ b/arch/arm/mach-mcs814x/Kconfig
> @@ -0,0 +1,11 @@
> +if ARCH_MCS814X
> +
> +config MCS8140
> + select CPU_ARM926T
> + bool
> +
> +menu "Moschip MCS8140 boards"
> +
> +endmenu
> +
> +endif
What values does the Kconfig file actually provide? I see that you are
adding the boards here later, but I think it would be nice to just
make the new platform a single Kconfig option with no individual
board selection.
> +struct clk {
> + struct clk *parent; /* parent clk */
> + unsigned long rate; /* clock rate in Hz */
> + unsigned long divider; /* clock divider */
> + u32 usecount; /* reference count */
> + struct clk_ops *ops; /* clock operation */
> + u32 enable_reg; /* clock enable register */
> + u32 enable_mask; /* clock enable mask */
> +};
Platforms are now converting to the common clock framework in drivers/clk.
Mike Turquette as the subsystem maintainer can probably judge better whether
we should still be allowing new platforms with their own implementation
of clk, but my feeling is that you should use the subsystem instead
and add a driver in a subdirectory of drivers/clk instead of in the
platform.
> +void __iomem *mcs814x_sysdbg_base;
Does this have to be globally visible? I would recommend making it static.
> +
> +struct cpu_mode {
> + const char *name;
> + int gpio_start;
> + int gpio_end;
> +};
> +
> +static const struct cpu_mode cpu_modes[] = {
> + {
> + .name = "I2S",
> + .gpio_start = 4,
> + .gpio_end = 8,
> + },
> + {
> + .name = "UART",
> + .gpio_start = 4,
> + .gpio_end = 9,
> + },
> + {
> + .name = "External MII",
> + .gpio_start = 0,
> + .gpio_end = 16,
> + },
> + {
> + .name = "Normal",
> + .gpio_start = -1,
> + .gpio_end = -1,
> + },
> +};
> +
> +void __init mcs814x_init_machine(void)
> +{
> + u32 bs2, cpu_mode;
> + int gpio;
> +
> + bs2 = __raw_readl(mcs814x_sysdbg_base + SYSDBG_BS2);
> + cpu_mode = (bs2 >> CPU_MODE_SHIFT) & CPU_MODE_MASK;
> +
> + pr_info("CPU mode: %s\n", cpu_modes[cpu_mode].name);
> +
> + /* request the gpios since the pins are muxed for functionnality */
> + for (gpio = cpu_modes[cpu_mode].gpio_start;
> + gpio == cpu_modes[cpu_mode].gpio_end; gpio++) {
> + if (gpio != -1)
> + gpio_request(gpio, cpu_modes[cpu_mode].name);
> + }
> +}
This looks like a very simple instance of a pinmux driver. I wonder
if it's worth doing an actual pinctrl driver that knows about these
modes and about the gpio handling of the platform. Maybe Linus Walleij
can comment on that.
How is the cpu mode managed? Is it hardwired through something like
strapping pins, or could you have a system that sets it dynamically
based on what hardware is being plugged in?
> +void __init mcs814x_map_io(void)
> +{
> + iotable_init(mcs814x_io_desc, ARRAY_SIZE(mcs814x_io_desc));
> +
> + mcs814x_sysdbg_base = ioremap(MCS814X_IO_START + MCS814X_SYSDBG,
> + MCS814X_SYSDBG_SIZE);
> + if (!mcs814x_sysdbg_base)
> + panic("unable to remap sysdbg base");
> +}
> +
> +void mcs814x_restart(char mode, const char *cmd)
> +{
> + __raw_writel(~(1 << 31), mcs814x_sysdbg_base);
> +}
You should generally avoid using __raw_readl etc. and instead use
readl/write or readl_relaxed/writel_relaxed.
> diff --git a/arch/arm/mach-mcs814x/common.h b/arch/arm/mach-mcs814x/common.h
> new file mode 100644
> index 0000000..e523abe
> --- /dev/null
> +++ b/arch/arm/mach-mcs814x/common.h
> @@ -0,0 +1,15 @@
> +#ifndef __ARCH_MCS814X_COMMON_H
> +#define __ARCH_MCS814X_COMMON_H
> +
> +#include <asm/mach/time.h>
> +
> +void mcs814x_map_io(void);
> +void mcs814x_clk_init(void);
> +void mcs814x_of_irq_init(void);
> +void mcs814x_init_machine(void);
> +void mcs814x_handle_irq(struct pt_regs *regs);
> +void mcs814x_restart(char mode, const char *cmd);
> +extern struct sys_timer mcs814x_timer;
> +extern void __iomem *mcs814x_sysdbg_base;
> +
> +#endif /* __ARCH_MCS814X_COMMON_H */
I think a lot of these don't actually need to be global if you combine some
of the files.
> diff --git a/arch/arm/mach-mcs814x/include/mach/cpu.h b/arch/arm/mach-mcs814x/include/mach/cpu.h
> new file mode 100644
> index 0000000..1ef3c4a
> --- /dev/null
> +++ b/arch/arm/mach-mcs814x/include/mach/cpu.h
> @@ -0,0 +1,16 @@
> +#ifndef __ASM_ARCH_CPU_H__
> +#define __ASM_ARCH_CPU_H__
> +
> +#include <asm/cputype.h>
> +
> +#define MCS8140_ID 0x41069260 /* ARM926EJ-S */
> +#define MCS814X_MASK 0xff0ffff0
> +
> +#ifdef CONFIG_MCS8140
> +/* Moschip MCS8140 is based on an ARM926EJ-S core */
> +#define soc_is_mcs8140() ((read_cpuid_id() & MCS814X_MASK) == MCS8140_ID)
> +#else
> +#define soc_is_mcs8140() (0)
> +#endif /* !CONFIG_MCS8140 */
> +
> +#endif /* __ASM_ARCH_CPU_H__ */
Can you explain what this is used for? Are there other SoCs in the same
platform that will get added later?
> diff --git a/arch/arm/mach-mcs814x/include/mach/hardware.h b/arch/arm/mach-mcs814x/include/mach/hardware.h
> new file mode 100644
> index 0000000..529f648
> --- /dev/null
> +++ b/arch/arm/mach-mcs814x/include/mach/hardware.h
> @@ -0,0 +1,16 @@
> +/*
> + * Copyright (C) 2003 Artec Design Ltd.
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __ASM_ARCH_HARDWARE_H
> +#define __ASM_ARCH_HARDWARE_H
> +
> +#include "mcs814x.h"
> +
> +#endif
I'd just make this an empty file, like gpio.h. That will let us kill off
these files more easily in the future.
> diff --git a/arch/arm/mach-mcs814x/include/mach/irqs.h b/arch/arm/mach-mcs814x/include/mach/irqs.h
> new file mode 100644
> index 0000000..78021d1
> --- /dev/null
> +++ b/arch/arm/mach-mcs814x/include/mach/irqs.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (C) 2003 Artec Design Ltd.
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __ASM_ARCH_IRQS_H
> +#define __ASM_ARCH_IRQS_H
> +
> +#define FIQ_START 0
Why do you need the FIQ_START macro here?
> +#define NR_IRQS 32
> +
> +#define IRQ_PCI_INTA 22
> +#define IRQ_PCI_INTB 23
> +#define IRQ_PCI_INTC 24
> +#define IRQ_PCI_INTD 26
The PCI interrupts should come from the device tree.
You should also select CONFIG_SPARSE_IRQ unconditionally and use IRQ domains
so that you no longer need to set NR_IRQS.
> diff --git a/arch/arm/mach-mcs814x/include/mach/mcs814x.h b/arch/arm/mach-mcs814x/include/mach/mcs814x.h
> new file mode 100644
> index 0000000..a4a369e
> --- /dev/null
> +++ b/arch/arm/mach-mcs814x/include/mach/mcs814x.h
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) 2003 Artec Design Ltd.
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __ASM_ARCH_MCS814X_H
> +#define __ASM_ARCH_MCS814X_H
> +
> +#define MCS814X_IO_BASE 0xF0000000
> +#define MCS814X_IO_START 0x40000000
> +#define MCS814X_IO_SIZE 0x00100000
What are these required for in the global space? Can you move them into
a single .c file?
> +/* IRQ controller register offset */
> +#define MCS814X_IRQ_ICR 0x00
> +#define MCS814X_IRQ_ISR 0x04
> +#define MCS814X_IRQ_MASK 0x20
> +#define MCS814X_IRQ_STS0 0x40
I'm pretty sure these can be in irq.c
> +#define MCS814X_PHYS_BASE 0x40000000
> +#define MCS814X_VIRT_BASE MCS814X_IO_BASE
> +
> +#define MCS814X_UART 0x000DC000
> +#define MCS814X_DBGLED 0x000EC000
> +#define MCS814X_SYSDBG 0x000F8000
> +#define MCS814X_SYSDBG_SIZE 0x50
> +
> +/* System configuration and bootstrap registers */
> +#define SYSDBG_BS1 0x00
> +#define CPU_FREQ_SHIFT 27
> +#define CPU_FREQ_MASK 0x0F
> +#define SDRAM_FREQ_BIT (1 << 22)
> +
> +#define SYSDBG_BS2 0x04
> +#define LED_CFG_MASK 0x03
> +#define CPU_MODE_SHIFT 23
> +#define CPU_MODE_MASK 0x03
> +
> +#define SYSDBG_SYSCTL_MAC 0x1d
> +#define BUF_SHIFT_BIT (1 << 0)
> +
> +#define SYSDBG_SYSCTL 0x08
> +#define SYSCTL_EMAC (1 << 0)
> +#define SYSCTL_EPHY (1 << 0) /* active low */
> +#define SYSCTL_CIPHER (1 << 16)
> +
> +#define SYSDBG_PLL_CTL 0x3C
The sysdbg stuff can probably go into a file that deals
with the sysdbg registers and exports a high-level interface.
> +
> +void __iomem *mcs814x_intc_base;
static?
> +#define MCS8140_PCI_HOST_BASE 0x80000000
> +#define MCS8140_PCI_IOMISC_BASE 0x00000000
> +#define MCS8140_PCI_PRE_BASE 0x10000000
> +#define MCS8140_PCI_NONPRE_BASE 0x30000000
> +
> +#define MCS8140_PCI_CFG_BASE (MCS8140_PCI_HOST_BASE + 0x04000000)
> +#define MCS8140_PCI_IO_BASE (MCS8140_PCI_HOST_BASE)
> +
> +#define MCS8140_PCI_IO_VIRT_BASE (MCS814X_IO_BASE - \
> + MCS8140_PCI_CONFIG_SIZE - \
> + MCS8140_PCI_IOMISC_SIZE)
> +#define MCS8140_PCI_CFG_VIRT_BASE (MCS814X_IO_BASE - \
> + MCS8140_PCI_CONFIG_SIZE)
> +static unsigned long __pci_addr(struct pci_bus *bus,
> + unsigned int devfn, int offset)
> +{
> + unsigned int busnr = bus->number;
> + unsigned int slot;
> +
> + /* we only support bus 0 */
> + if (busnr != 0)
> + return 0;
Why? A lot of devices have bridges on them and they should just
work if you take out this check.
> + /*
> + * Trap out illegal values
> + */
> + BUG_ON(devfn > 255 || busnr > 255 || devfn > 255);
You're checking devfn twice...
> + /* Scan 3 slots */
> + slot = PCI_SLOT(devfn);
> + switch (slot) {
> + case 1:
> + case 2:
> + case 3:
> + if (PCI_FUNC(devfn) >= 4)
> + return 0;
This also looks bogus. Why limit it to three devices?
> +static int mcs8140_pci_read_config(struct pci_bus *bus,
> + unsigned int devfn, int where,
> + int size, u32 *val)
> +{
> + unsigned long v = 0xFFFFFFFF;
> + unsigned long addr = __pci_addr(bus, devfn, where);
> +
> + if (addr != 0) {
> + switch (size) {
> + case 1:
> + v = __raw_readb(addr);
> + break;
> + case 2:
> + addr &= ~1;
> + v = __raw_readw(addr);
> + break;
> + default:
> + addr &= ~3;
> + v = __raw_readl(addr);
> + break;
> + }
> + } else
> + v = 0xffffffff;
This is just mmconfig, right? Don't we have a generic implementation for that?
> +
> +static struct resource io_mem = {
> + .name = "PCI I/O space",
> + .start = MCS8140_PCI_HOST_BASE + MCS8140_PCI_IOMISC_BASE,
> + .end = MCS8140_PCI_HOST_BASE + MCS8140_PCI_IOMISC_BASE + SZ_64M,
> + .flags = IORESOURCE_IO,
> +};
This is wrong: MCS8140_PCI_HOST_BASE is an address in IORESOURCE_MEM space.
You probably also mean SZ_64K instead of SZ_64M.
> +int __init pci_mcs8140_setup(int nr, struct pci_sys_data *sys)
> +{
> + int ret = 0;
> + u32 val;
> +
> + if (nr > 0)
> + return 0;
> +
> + sys->mem_offset = MCS8140_PCI_IO_VIRT_BASE - MCS8140_PCI_IO_BASE;
Your address spaces are very confusing (or confused). So you have defined
#define MCS8140_PCI_IO_VIRT_BASE (MCS814X_IO_BASE - \
MCS8140_PCI_CONFIG_SIZE - \
MCS8140_PCI_IOMISC_SIZE)
#define MCS8140_PCI_CFG_VIRT_BASE (MCS814X_IO_BASE - \
MCS8140_PCI_CONFIG_SIZE)
which means you mem_offset is (MCS814X_IO_BASE - MCS8140_PCI_CONFIG_SIZE -
MCS8140_PCI_IOMISC_SIZE) - (MCS814X_IO_BASE - MCS8140_PCI_CONFIG_SIZE)
which is (-MCS8140_PCI_IOMISC_SIZE), which in turn evaluates to
-SZ_64M or 0xfc000000. If that is the correct value, I'm sure it's
just coincidence ;-)
> +static int __init mcs8140_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> +{
> + int line = IRQ_PCI_INTA;
> +
> + if (pin != 0) {
> + /* IRQ_PCIA - 22 */
> + if (pin == PCI_INTD)
> + line = IRQ_PCI_INTA + pin; /* IRQ_PCIA - 22 */
> + else
> + line = IRQ_PCI_INTA + pin - 1; /* IRQ_PCIA - 22 */
> + }
> +
> + pr_info("PCI: Map interrupt slot 0x%02x pin 0x%02x line 0x%02x\n",
> + slot, pin, line);
> +
> + return line;
> +}
This looks like a very unusual mapping. Maybe it's just wrong and that
explains why you don't support child buses or additional slots?
In any case, you should use an "interrupt-map" in the device tree that
describes how the IRQs are mapped to the slots. I think we have generic
code to deal with that already.
> +static struct map_desc mcs8140_pci_io_desc[] __initdata = {
> + {
> + .virtual = MCS8140_PCI_CFG_VIRT_BASE,
> + .pfn = __phys_to_pfn(MCS8140_PCI_CFG_BASE),
> + .length = MCS8140_PCI_CONFIG_SIZE,
> + .type = MT_DEVICE
> + },
> + {
> + .virtual = MCS8140_PCI_IO_VIRT_BASE,
> + .pfn = __phys_to_pfn(MCS8140_PCI_IO_BASE),
> + .length = MCS8140_PCI_IOMISC_SIZE,
> + .type = MT_DEVICE
> + },
> +};
Please have a look at the latest patches from Rob Herring about
mapping the I/O space. I don't think you need to map the config
space early because you don't rely on a fixed location for that.
Just use of_iomap() to find that and store the pointer to the
config space somewhere.
> + pcibios_min_io = MCS8140_PCI_HOST_BASE;
Leave this one at the default of 0x1000. MCS8140_PCI_HOST_BASE is the
address from the CPU's point of view, not from the bus. You just need
to stay out of the range of possible legacy ISA devices.
It's probably a good idea to keep the PCI support as a separate patch
for reviewing purposes.
Arnd
More information about the devicetree-discuss
mailing list