Re: [Qemu-devel] [PATCH v5 04/11] ARM: exynos4210: IRQ subsystem support.

[Peter Maydell]

On 23 December 2011 11:40, Evgeny Voevodin <address@hidden> wrote:
>
> Signed-off-by: Evgeny Voevodin <address@hidden>
> ---
>  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 <address@hidden>
> + *
> + * 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