On 23 December 2011 11:40, Evgeny Voevodin <e.voevo...@samsung.com> wrote:
>
> Signed-off-by: Evgeny Voevodin <e.voevo...@samsung.com>
> ---
> Makefile.target | 3 +-
> hw/exynos4210.c | 179 ++++++++++++++++++-
> hw/exynos4210.h | 46 +++++
> hw/exynos4210_combiner.c | 384 ++++++++++++++++++++++++++++++++++++++++
> hw/exynos4210_gic.c | 437
> ++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 1044 insertions(+), 5 deletions(-)
> create mode 100644 hw/exynos4210_combiner.c
> create mode 100644 hw/exynos4210_gic.c
>
> diff --git a/Makefile.target b/Makefile.target
> index 0246947..ccd1f79 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -336,7 +336,8 @@ obj-arm-y = integratorcp.o versatilepb.o arm_pic.o
> arm_timer.o
> obj-arm-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o
> pl190.o
> obj-arm-y += versatile_pci.o
> obj-arm-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o
> -obj-arm-y += exynos4_boards.o exynos4210.o exynos4210_uart.o
> +obj-arm-y += exynos4_boards.o exynos4210.o exynos4210_uart.o
> exynos4210_gic.o \
> + exynos4210_combiner.o
Don't use the '\' line continuation, just start a new obj-arm-y += line.
> + for (n = 0; n < max; n++) {
> +
> + bit = EXYNOS4210_COMBINER_GET_BIT_NUM(n);
> +
> + switch (n) {
> + /* MDNIE_LCD1 INTG1*/
Space before the "*/", please.
> + case EXYNOS4210_COMBINER_GET_IRQ_NUM(1, 0) ...
> + EXYNOS4210_COMBINER_GET_IRQ_NUM(1, 3):
> + irq[n] = qemu_irq_split(qdev_get_gpio_in(dev, n),
> + irq[EXYNOS4210_COMBINER_GET_IRQ_NUM(0, bit + 4)]);
> + continue;
> + break;
The 'break' here is unreachable, right?
Also the indentation is off: the irq[n] line should start 4 chars
in from the 'case', not 8, and the 'continue' or 'break' should
too.
> @@ -112,16 +279,20 @@ Exynos4210State *exynos4210_init(MemoryRegion
> *system_mem,
>
> /*** UARTs ***/
> exynos4210_uart_create(EXYNOS4210_UART0_BASE_ADDR,
> - EXYNOS4210_UART0_FIFO_SIZE, 0, NULL, NULL);
> + EXYNOS4210_UART0_FIFO_SIZE, 0, NULL,
> + irq_table[exynos4210_get_irq(EXYNOS4210_UART_INT_GRP,
> 0)]);
Having to modify the UART creation to add the IRQ line later suggests
this patch series isn't in quite the right order. My suggestion:
1. hw/sysbus.h: Increase maximum number of device IRQs
2. hw/arm_boot.c: Extend secondary CPU bootloader.
3. ARM: exynos4210: IRQ subsystem support.
4. ARM: Samsung exynos4210-based boards emulation
[fold hw/exynos4210.c: Boot secondary CPU. into this patch]
5. ARM: exynos4210: UART support
6. ARM: exynos4210: PWM support.
7. hw/lan9118: Add basic 16-bit mode support.
8. hw/exynos4210.c: Add LAN support for SMDKC210.
9. Exynos4210: added display controller implementation
> +++ b/hw/exynos4210_combiner.c
> @@ -0,0 +1,384 @@
> +/*
> + * Samsung exynos4210 Interrupt Combiner
> + *
> + * Copyright (c) 2000 - 2011 Samsung Electronics Co., Ltd.
> + * All rights reserved.
> + *
> + * Evgeny Voevodin <e.voevo...@samsung.com>
> + *
> + * 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; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
I think it would be good to have a paragraph here explaining what
a combiner actually is, for the benefit of people not familiar
with the SoC.
> +static const MemoryRegionOps exynos4210_combiner_ops = {
> + .read = exynos4210_combiner_read,
> + .write = exynos4210_combiner_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
Indentation is wrong here.
> +
> +/*
> + * Internal Combiner initialization.
> + */
> +static int exynos4210_combiner_init(SysBusDevice *dev)
> +{
> + unsigned int i;
> + struct Exynos4210CombinerState *s =
> + FROM_SYSBUS(struct Exynos4210CombinerState, dev);
> +
> + /* Allocate general purpose input signals and connect a handler to each
> of
> + * them */
> + qdev_init_gpio_in(&s->busdev.qdev, exynos4210_combiner_handler,
> IIC_NIRQ);
> +
> + /* Connect SysBusDev irqs to device specific irqs */
> + for (i = 0; i < IIC_NIRQ; i++) {
> + sysbus_init_irq(dev, &s->output_irq[i]);
> + }
> +
> + memory_region_init_io(&s->iomem, &exynos4210_combiner_ops, s,
> + "exynos4210-combiner", IIC_REGION_SIZE);
> + sysbus_init_mmio(dev, &s->iomem);
> +
> + exynos4210_combiner_reset((DeviceState *)s);
You don't need to explicitly reset in the qdev init function if you've
registered the reset function as qdev.reset (as you have).
> + return 0;
> +}
> +
> +static SysBusDeviceInfo exynos4210_combiner_info = {
> + .qdev.name = "exynos4210.combiner",
> + .qdev.size = sizeof(struct Exynos4210CombinerState),
> + .qdev.reset = exynos4210_combiner_reset,
> + .qdev.vmsd = &VMState_Exynos4210Combiner,
> + .init = exynos4210_combiner_init,
> + .qdev.props = (Property[]) {
> + DEFINE_PROP_UINT32("external",
> + struct Exynos4210CombinerState,
> + external,
> + 0),
> + DEFINE_PROP_END_OF_LIST(),
> + }
> +};
More dodgy indentation -- can you go through and do a general check,
please?
> + memory_region_add_subregion(&s->cpu_container,
> + EXYNOS4210_EXT_GIC_CPU_GET_OFFSET(i), &s->cpu_alias[i]);
> +
> + /* Map Distributer per SMP Core */
"Distributor".
-- PMM
