Subject: [PATCH 2/8] arm: mach-armada: add source files

From: [email-masked] (Gregory CLEMENT)
Date: Tue, 15 May 2012 11:59:06 +0200

On 05/15/2012 11:46 AM, Ben Dooks wrote:
> On 15/05/12 09:54, Thomas Petazzoni wrote:
>> This patch adds basic source files for Marvell Armada SoCs.
>>
>> Signed-off-by: Gregory CLEMENT<[email-masked]>
>> Signed-off-by: Thomas Petazzoni<[email-masked]>
>> Signed-off-by: Lior Amsalem<[email-masked]>
>> ---
>>   arch/arm/boot/dts/a370.dtsi        |   23 ++++
>>   arch/arm/boot/dts/armada.dtsi      |   67 ++++++++++
>>   arch/arm/boot/dts/axp.dtsi         |   43 +++++++
>>   arch/arm/mach-armada/Kconfig       |    5 +
>>   arch/arm/mach-armada/Makefile      |    2 +
>>   arch/arm/mach-armada/Makefile.boot |    1 +
>>   arch/arm/mach-armada/common.c      |   56 +++++++++
>>   arch/arm/mach-armada/common.h      |   27 ++++
>>   arch/arm/mach-armada/irq.c         |  116 +++++++++++++++++
>>   arch/arm/mach-armada/time.c        |  243 ++++++++++++++++++++++++++++++++++++
>>   10 files changed, 583 insertions(+)
>>   create mode 100644 arch/arm/boot/dts/a370.dtsi
>>   create mode 100644 arch/arm/boot/dts/armada.dtsi
>>   create mode 100644 arch/arm/boot/dts/axp.dtsi
>>   create mode 100644 arch/arm/mach-armada/Kconfig
>>   create mode 100644 arch/arm/mach-armada/Makefile
>>   create mode 100644 arch/arm/mach-armada/Makefile.boot
>>   create mode 100644 arch/arm/mach-armada/common.c
>>   create mode 100644 arch/arm/mach-armada/common.h
>>   create mode 100644 arch/arm/mach-armada/irq.c
>>   create mode 100644 arch/arm/mach-armada/time.c
>>
>> diff --git a/arch/arm/boot/dts/a370.dtsi b/arch/arm/boot/dts/a370.dtsi
>> new file mode 100644
>> index 0000000..f11e56a
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/a370.dtsi
>> @@ -0,0 +1,23 @@
>> +/*
>> + * Device Tree Include file for Marvell Armada 370 family SoC
>> + *
>> + * Copyright (C) 2012 Marvell
>> + *
>> + * Lior Amsalem<[email-masked]>
>> + * Gregory CLEMENT<[email-masked]>
>> + * Thomas Petazzoni<[email-masked]>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2.  This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + *
>> + * Contains definitions specific to the Armada 370 SoC that are not
>> + * common to all Armada SoCs.
>> + */
>> +
>> +/include/ "armada.dtsi"
> 
> I would use armada_xp.dtsi at the least, we've no way of knowing what
> further Armada devices are going to be produced and whether they will be
> compatible with.
> 
>> +/ {
>> +	model = "Marvell Armada 370 family SoC";
>> +	compatible = "marvell,armada370", "marvell,armada";
>> + };
> 
> Firstly, it is mrvl, not marvell everywhere else in the kernel (see
> MIPS and device tree documentation).
> 
> Secondly, I would strongly advise against using the generic marketing
> name for these devices in the compatible list, "marvell,armada" as we
> have no way of knowing what new devices will come along in the future
> and if they'll be compatible.
> 
>> diff --git a/arch/arm/boot/dts/armada.dtsi b/arch/arm/boot/dts/armada.dtsi
>> new file mode 100644
>> index 0000000..3c99c30
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/armada.dtsi
>> @@ -0,0 +1,67 @@
>> +/*
>> + * Device Tree Include file for Marvell Armada family SoC
>> + *
>> + * Copyright (C) 2012 Marvell
>> + *
>> + * Lior Amsalem<[email-masked]>
>> + * Gregory CLEMENT<[email-masked]>
>> + * Thomas Petazzoni<[email-masked]>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2.  This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + *
>> + * This file contains the definitions that are common to the Armada
>> + * 370 and Armada XP SoC.
> 
> I really do not want to see these files in the kernel, we where suppose
> to remove any specific manchine dependency by moving to device tree and
> not push the problem from a board.c to a .dts file.
> 
> Given these really should be part of the bootloader, I am not sure if a
> GPLv2 compliant license is really appropriate for these files. At best
> some form of dual-licensing to allow them to be moved out into a devtree
> repository elsewhere would be useful.
> 
> (Feedback from Grant here would be useful)
> 
> PS, given the amount of devtree related stuff, CC'ing Grant Likely may
> have been useful./
> 
>> + */
>> +
>> +/include/ "skeleton.dtsi"
>> +
>> +/ {
>> +	model = "Marvell Armada family SoC";
>> +	compatible = "marvell,armada";
>> +
>> +	cpus {
>> +		cpu at 0 {
>> +			compatible = "marvell,sheeva-v7";
>> +		};
>> +	};
>> +
>> +	mpic: interrupt-controller at d0020000 {
>> +	      compatible = "marvell,mpic";
> 
> Should this be "mrvl,axp_mpic" ?
> 
>> +	      #interrupt-cells =<1>;
>> +	      #address-cells =<1>;
>> +	      #size-cells =<1>;
>> +	      interrupt-controller;
>> +	      reg =<0xd0020000 0x1000>,
>> +		<0xd0021000 0x1000>;
>> +	};
>> +
>> +	soc {
>> +		#address-cells =<1>;
>> +		#size-cells =<1>;
>> +		compatible = "simple-bus";
>> +		interrupt-parent =<&mpic>;
>> +		ranges;
>> +
>> +		serial at d0012000 {
>> +				compatible = "ns16550";
> 
> thought these where more akin to the designware uart blocks?
> 
>> +				reg =<0xd0012000 0x100>;
>> +				reg-shift =<2>;
>> +				interrupts =<41>;
>> +		};
>> +		serial at d0012100 {
>> +				compatible = "ns16550";
>> +				reg =<0xd0012100 0x100>;
>> +				reg-shift =<2>;
>> +				interrupts =<42>;
>> +		};
>> +
>> +		timer at d0020300 {
>> +			       compatible = "marvell,timer";
> 
> Is this really a generic marvell timer block?
> 
>> +			       reg =<0xd0020300 0x30>;
>> +			       interrupts =<37>,<38>,<39>,<40>;
>> +		};
>> +	};
>> +};
>> +
>> diff --git a/arch/arm/boot/dts/axp.dtsi b/arch/arm/boot/dts/axp.dtsi
>> new file mode 100644
>> index 0000000..6427268
>> --- /dev/null
> 
> [snip, pretty much the same comments here.
> 
>> diff --git a/arch/arm/mach-armada/Kconfig b/arch/arm/mach-armada/Kconfig
>> new file mode 100644
>> index 0000000..010a6a3
>> --- /dev/null
>> +++ b/arch/arm/mach-armada/Kconfig
>> @@ -0,0 +1,5 @@
>> +if ARCH_ARMADA
>> +
>> +menu "Marvell Armada Implementations"
>> +
>> +endif
>> diff --git a/arch/arm/mach-armada/Makefile b/arch/arm/mach-armada/Makefile
>> new file mode 100644
>> index 0000000..6765961
>> --- /dev/null
>> +++ b/arch/arm/mach-armada/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-y += common.o irq.o time.o
>> +
>> diff --git a/arch/arm/mach-armada/Makefile.boot b/arch/arm/mach-armada/Makefile.boot
>> new file mode 100644
>> index 0000000..b327175
>> --- /dev/null
>> +++ b/arch/arm/mach-armada/Makefile.boot
>> @@ -0,0 +1 @@
>> +zreladdr-y := 0x00008000
>> diff --git a/arch/arm/mach-armada/common.c b/arch/arm/mach-armada/common.c
>> new file mode 100644
>> index 0000000..92d9ec2
>> --- /dev/null
>> +++ b/arch/arm/mach-armada/common.c
>> @@ -0,0 +1,56 @@
>> +/*
>> + * Core functions for Marvell Armada System On Chip
>> + *
>> + * Copyright (C) 2012 Marvell
>> + *
>> + * Lior Amsalem<[email-masked]>
>> + * Gregory CLEMENT<[email-masked]>
>> + * Thomas Petazzoni<[email-masked]>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2.  This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#include<linux/kernel.h>
>> +#include<linux/delay.h>
>> +#include<linux/init.h>
>> +#include<linux/platform_device.h>
>> +#include<linux/irq.h>
>> +#include<linux/timex.h>
>> +#include<asm/page.h>
>> +#include<asm/setup.h>
>> +#include<asm/mach/map.h>
>> +#include<plat/addr-map.h>
>> +#include<mach/armada.h>
>> +#include<mach/bridge-regs.h>
>> +#include "common.h"
> 
> for some reason there are no spaces between #include and the rest
> of the code?
> 
>> +
>> +static struct map_desc armada_io_desc[] __initdata = {
>> +	{
>> +		.virtual	= ARMADA_REGS_VIRT_BASE,
>> +		.pfn		= __phys_to_pfn(ARMADA_REGS_PHYS_BASE),
>> +		.length		= ARMADA_REGS_SIZE,
>> +		.type		= MT_DEVICE,
>> +	},
>> +};
>> +
>> +void __init armada_map_io(void)
>> +{
>> +	iotable_init(armada_io_desc, ARRAY_SIZE(armada_io_desc));
>> +}
>> +
>> +void armada_restart(char mode, const char *cmd)
>> +{
>> +	/*
>> +	 * Enable soft reset to assert RSTOUTn.
>> +	 */
>> +	writel(SOFT_RESET_OUT_EN, RSTOUTn_MASK);
>> +
>> +	/*
>> +	 * Assert soft reset.
>> +	 */
>> +	writel(SOFT_RESET, SYSTEM_SOFT_RESET);
>> +	while (1)
>> +		;
>> +}
>> diff --git a/arch/arm/mach-armada/common.h b/arch/arm/mach-armada/common.h
>> new file mode 100644
>> index 0000000..e453161
>> --- /dev/null
>> +++ b/arch/arm/mach-armada/common.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Core functions for Marvell Armada System On Chip
>> + *
>> + * Copyright (C) 2012 Marvell
>> + *
>> + * Lior Amsalem<[email-masked]>
>> + * Gregory CLEMENT<[email-masked]>
>> + * Thomas Petazzoni<[email-masked]>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2.  This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#ifndef __ARCH_ARMADA_COMMON_H
>> +#define __ARCH_ARMADA_COMMON_H
>> +
>> +#include<asm/exception.h>
>> +
>> +extern struct sys_timer armada_timer;
>> +
>> +void armada_map_io(void);
>> +void armada_init_irq(void);
>> +void armada_restart(char, const char *);
>> +asmlinkage void __exception_irq_entry armada_handle_irq(struct pt_regs *regs);
>> +
>> +#endif
>> diff --git a/arch/arm/mach-armada/irq.c b/arch/arm/mach-armada/irq.c
>> new file mode 100644
>> index 0000000..7006429
>> --- /dev/null
>> +++ b/arch/arm/mach-armada/irq.c
>> @@ -0,0 +1,116 @@
>> +/*
>> + * Marvall Armada SoC IRQ handling
>> + *
>> + * Copyright (C) 2012 Marvell
>> + *
>> + * Lior Amsalem<[email-masked]>
>> + * Gregory CLEMENT<[email-masked]>
>> + * Thomas Petazzoni<[email-masked]>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2.  This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#include<linux/kernel.h>
>> +#include<linux/init.h>
>> +#include<linux/irq.h>
>> +#include<linux/interrupt.h>
>> +#include<linux/io.h>
>> +#include<linux/of_address.h>
>> +#include<linux/of_irq.h>
>> +#include<linux/irqdomain.h>
>> +#include<asm/mach/arch.h>
>> +#include<asm/exception.h>
>> +
>> +/* Interrupt Controller Registers Map */
>> +#define ARMADA_INT_SET_MASK_OFFS             (0xBC)
>> +#define ARMADA_INT_CLEAR_MASK_OFFS           (0xB8)
>> +
>> +#define ARMADA_INT_SET_ENABLE_OFFS           (0xA30)
>> +#define ARMADA_INT_CLEAR_ENABLE_OFFS         (0xA34)
>> +
>> +#define ARMADA_CPU_INTACK_OFFS               (0xB4)
> 
> MPIC_ since you indicated earlier that it is pretty generic.
> 
>> +
>> +static void __iomem *per_cpu_int_base;
>> +static void __iomem *main_int_base;
>> +static struct irq_domain *armada_mpic_domain;
>> +
>> +static void armada_irq_mask(struct irq_data *d)
>> +{
>> +	writel(d->irq, per_cpu_int_base + ARMADA_INT_CLEAR_MASK_OFFS);
>> +}
>> +
>> +static void armada_irq_unmask(struct irq_data *d)
>> +{
>> +	writel(d->irq, per_cpu_int_base + ARMADA_INT_SET_MASK_OFFS);
>> +}
>> +
>> +static struct irq_chip armada_irq_chip = {
>> +	.name		= "armada_irq",
>> +	.irq_mask       = armada_irq_mask,
>> +	.irq_mask_ack   = armada_irq_mask,
>> +	.irq_unmask     = armada_irq_unmask,
>> +};
>> +
>> +static int armada_mpic_irq_map(struct irq_domain *h,
>> +			       unsigned int virq,
>> +			       irq_hw_number_t hw)
>> +{
>> +	armada_irq_mask(irq_get_irq_data(virq));
>> +	writel(virq, main_int_base + ARMADA_INT_SET_ENABLE_OFFS);
>> +
>> +	irq_set_chip_and_handler(virq,&armada_irq_chip,
>> +				 handle_level_irq);
>> +	irq_set_status_flags(virq, IRQ_LEVEL);
>> +	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
>> +	return 0;
> 
> you could have used the per-irq driver private data to keep
> things like the base of controller in.
> 
>> +}
>> +
>> +static struct irq_domain_ops armada_mpic_irq_ops = {
>> +	.map = armada_mpic_irq_map,
>> +	.xlate = irq_domain_xlate_onecell,
>> +};
>> +
>> +static void __init armada_mpic_of_init(struct device_node *node,
>> +				       struct device_node *parent)
>> +{
>> +	main_int_base = of_iomap(node, 0);
>> +	per_cpu_int_base = of_iomap(node, 1);
> 
> Please verify the return values.
> 
>> +
>> +	armada_mpic_domain = irq_domain_add_linear(node, NR_IRQS,
>> +						&armada_mpic_irq_ops, NULL);
>> +	if (!armada_mpic_domain)
>> +		panic("Unable to add Armada MPIC irq domain (DT)\n");
>> +
>> +	irq_set_default_host(armada_mpic_domain);
>> +}
>> +
>> +asmlinkage void __exception_irq_entry armada_handle_irq(struct pt_regs *regs)
>> +{
>> +	u32 irqstat, irqnr;
>> +
>> +	do {
>> +		irqstat = readl_relaxed(per_cpu_int_base +
>> +					ARMADA_CPU_INTACK_OFFS);
>> +		irqnr = irqstat&  0x3FF;
>> +
>> +		if (irqnr<  1023) {
>> +			irqnr = irq_find_mapping(armada_mpic_domain, irqnr);
>> +			handle_IRQ(irqnr, regs);
>> +			continue;
>> +		}
>> +
>> +		break;
>> +	} while (1);
> 
> no timeout to get out of handling many IRQs?
> 
>> +}
>> +
>> +static const struct of_device_id mpic_of_match[] __initconst = {
>> +	{ .compatible = "marvell,mpic", .data = armada_mpic_of_init },
>> +	{},
>> +};
>> +
>> +void __init armada_init_irq(void)
>> +{
>> +	of_irq_init(mpic_of_match);
>> +}
>> diff --git a/arch/arm/mach-armada/time.c b/arch/arm/mach-armada/time.c
>> new file mode 100644
>> index 0000000..73817ea
>> --- /dev/null
>> +++ b/arch/arm/mach-armada/time.c
>> @@ -0,0 +1,243 @@
>> +/*
>> + * Marvell Armada SoC timer handling.
>> + *
>> + * Copyright (C) 2012 Marvell
>> + *
>> + * Lior Amsalem<[email-masked]>
>> + * Gregory CLEMENT<[email-masked]>
>> + * Thomas Petazzoni<[email-masked]>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2.  This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + *
>> + * Timer 0 is used as free-running clocksource, while timer 1 is
>> + * used as clock_event_device.
>> + */
>> +
>> +#include<linux/kernel.h>
>> +#include<linux/timer.h>
>> +#include<linux/clockchips.h>
>> +#include<linux/interrupt.h>
>> +#include<linux/of.h>
>> +#include<linux/of_irq.h>
>> +#include<linux/of_address.h>
>> +#include<linux/irq.h>
>> +#include<asm/sched_clock.h>
>> +#include<asm/mach/time.h>
> 
> Style issue again.
> 
>> +
>> +/*
>> + * Timer block registers.
>> + */
>> +#define TIMER_CTRL_OFF		0x0000
>> +#define  TIMER0_EN		 0x0001
>> +#define  TIMER0_RELOAD_EN	 0x0002
>> +#define  TIMER0_25MHZ            0x0800
>> +#define  TIMER0_DIV(div)         ((div)<<  19)
>> +#define  TIMER1_EN		 0x0004
>> +#define  TIMER1_RELOAD_EN	 0x0008
>> +#define  TIMER1_25MHZ            0x1000
>> +#define  TIMER1_DIV(div)         ((div)<<  22)
>> +#define TIMER_EVENTS_STATUS	0x0004
>> +#define  TIMER0_CLR_MASK         (~0x1)
>> +#define  TIMER1_CLR_MASK         (~0x100)
>> +#define TIMER0_RELOAD_OFF	0x0010
>> +#define TIMER0_VAL_OFF		0x0014
>> +#define TIMER1_RELOAD_OFF	0x0018
>> +#define TIMER1_VAL_OFF		0x001c
>> +
>> +/* Global timers are connected to the coherency fabric clock, and the
>> +   below divider reduces their incrementing frequency. */
>> +#define TIMER_DIVIDER_SHIFT     5
>> +#define TIMER_DIVIDER           (1<<  TIMER_DIVIDER_SHIFT)
>> +
>> +/*
>> + * SoC-specific data.
>> + */
>> +static void __iomem *timer_base;
>> +static int timer_irq;
>> +
>> +/*
>> + * Number of timer ticks per jiffy.
>> + */
>> +static u32 ticks_per_jiffy;
>> +
>> +static u32 notrace armada_read_sched_clock(void)
>> +{
>> +	return ~readl(timer_base + TIMER0_VAL_OFF);
>> +}
>> +
>> +/*
>> + * Clockevent handling.
>> + */
>> +static int
>> +armada_clkevt_next_event(unsigned long delta, struct clock_event_device *dev)
>> +{
>> +	unsigned long flags;
>> +	u32 u;
>> +
>> +	if (delta == 0)
>> +		return -ETIME;
>> +
>> +	local_irq_save(flags);
>> +
>> +	/*
>> +	 * Clear clockevent timer interrupt.
>> +	 */
>> +	writel(TIMER1_CLR_MASK, timer_base + TIMER_EVENTS_STATUS);
>> +
>> +	/*
>> +	 * Setup new clockevent timer value.
>> +	 */
>> +	writel(delta, timer_base + TIMER1_VAL_OFF);
>> +
>> +	/*
>> +	 * Enable the timer.
>> +	 */
>> +	u = readl(timer_base + TIMER_CTRL_OFF);
>> +	u = ((u&  ~TIMER1_RELOAD_EN) | TIMER1_EN |
>> +	     TIMER1_DIV(TIMER_DIVIDER_SHIFT));
>> +	writel(u, timer_base + TIMER_CTRL_OFF);
>> +
>> +	local_irq_restore(flags);
>> +
>> +	return 0;
> 
> 
> you could have halved the length by just using single line comments.

Right

> 
>> +}
>> +
>> +static void
>> +armada_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev)
>> +{
>> +	unsigned long flags;
>> +	u32 u;
>> +
>> +	local_irq_save(flags);
>> +	if (mode == CLOCK_EVT_MODE_PERIODIC) {
>> +		/*
>> +		 * Setup timer to fire at 1/HZ intervals.
>> +		 */
>> +		writel(ticks_per_jiffy - 1, timer_base + TIMER1_RELOAD_OFF);
>> +		writel(ticks_per_jiffy - 1, timer_base + TIMER1_VAL_OFF);
>> +
>> +		/*
>> +		 * Enable timer.
>> +		 */
>> +		u = readl(timer_base + TIMER_CTRL_OFF);
>> +
>> +		writel((u | TIMER1_EN | TIMER1_RELOAD_EN |
>> +			TIMER1_DIV(TIMER_DIVIDER_SHIFT)),
>> +		       timer_base + TIMER_CTRL_OFF);
>> +	} else {
>> +		/*
>> +		 * Disable timer.
>> +		 */
>> +		u = readl(timer_base + TIMER_CTRL_OFF);
>> +		writel(u&  ~TIMER1_EN, timer_base + TIMER_CTRL_OFF);
>> +
>> +		/*
>> +		 * ACK pending timer interrupt.
>> +		 */
>> +		writel(TIMER1_CLR_MASK,
>> +		       timer_base + TIMER_EVENTS_STATUS);
>> +
>> +	}
> 
> How about handling the one-shot case that you advertise below?
> 
>> +static void __init armada_time_init(void)
>> +{
>> +	u32 u;
>> +	struct device_node *np;
>> +	unsigned int timer_clk;
>> +
>> +	np = of_find_compatible_node(NULL, NULL, "marvell,timer");
>> +	timer_base = of_iomap(np, 0);
>> +	WARN_ON(!timer_base);
>> +
>> +	if (of_find_property(np, "marvell,timer-25Mhz", NULL)) {
>> +		/* The fixed 25MHz timer is available so let's use it*/
>> +		u = readl(timer_base + TIMER_CTRL_OFF);
>> +		writel(u | TIMER0_25MHZ | TIMER1_25MHZ,
>> +		       timer_base + TIMER_CTRL_OFF);
>> +		timer_clk = 25000000;
>> +	} else {
>> +		u32 clk;
>> +		of_property_read_u32(np, "clock-frequency",&clk);
>> +		WARN_ON(!clk);
>> +		u = readl(timer_base + TIMER_CTRL_OFF);
>> +		writel(u&  ~(TIMER0_25MHZ | TIMER1_25MHZ),
>> +		       timer_base + TIMER_CTRL_OFF);
>> +		timer_clk = clk / TIMER_DIVIDER;
>> +	}
> 
> Give we've not got clk_() support yet, should you just force 25M 
> operation until clk_get_rate() is supported?
> 
This code support Aramade XP _and_ Armada 370, and for Armada 370
there is not such feature available.

> [snip]

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
+33 602 196 044