[PATCH 1/8] ARM: support for Moschip MCS814x SoCs
Florian Fainelli
florian at openwrt.org
Tue Jul 17 05:34:15 EDT 2012
Hi Arnd,
On Monday 16 July 2012 15:54:04 Arnd Bergmann wrote:
> 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.
I plan on adding support for MCS8142 later on, thus I thought it would be nice
to be able to selectively compile board support for MCS8140 or some later
chip.
>
> > +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.
Yes, since it is used both by clock.c and common.c
>
> > +
> > +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?
Like I replied to Thomas, these settings are actually hardwired through
bootstrap registers, so I should not even bother to request these GPIOs.
>
> > +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.
Allright, even on an ARM926-EJS-based system?
>
> > 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?
Yes, that's the plan.
>
> > 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.
Ok.
>
> > 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?
I don't think I need this anymore.
>
> > +#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.
Ok.
>
> > 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?
I can certainly do this.
>
> > +/* 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
Not unless I switch to MULTI_IRQ_HANDLER.
>
> > +#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?
Since I am not yet using MULTI_IRQ_HANDLER, this is used by the entry-macro.S
file too.
>
> > +#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.
I will double check with the hardware manual, but I really think it is
64MBytes.
>
> > +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.
Good idea, I will do that. Note that none of the boards I have actually have
PCI, so the code is just there in case someone needs it. I could also go along
and remove it, since I cannot test it, which explains why there are so many
inconsistencies with it.
>
> Arnd
More information about the linux-arm-kernel
mailing list