Re: [Qemu-devel] [PATCH V2 2/3] exynos4210: add exynos4210 GPIO implementation

[Igor Mitsyanko <i.mitsyanko@samsung.com>]

On 03/20/2012 05:27 PM, Peter Maydell wrote:
> On 15 March 2012 07:35, Igor Mitsyanko<i.mitsyanko@samsung.com>  wrote:
>> Now that we have GPIO emulation for exynos4210 SoC we can use it to
>> properly hook up IRQ line to lan9215 controller on SMDK board.
>
>> +#elif EXYNOS4210_GPIO_DEBUG == 1
>> +#define DPRINT_L1(fmt, args...)       \
>> +    do {fprintf(stderr, "QEMU GPIO: "fmt, ## args); } while (0)
>
> Space after the closing quote in these macros (this patch and
> others), please.
>
>> +
>> +    DPRINT_L1("Input pin GPIO%s_PIN%u %s by external device\n",
>> +            g->ports[group_num].name, pin, (level ? "set" : "cleared"));
>> +    /* Set new value on corresponding gpio pin */
>> +    (level) ? (g->ports[group_num].dat |= (1<<  pin)) :
>> +            (g->ports[group_num].dat&= ~(1<<  pin));
>
> Don't use ?: like this please, just write it out as an if statement.
>

OK

>> +static void exynos4_gpio_write(void *opaque, target_phys_addr_t off,
>> +                               uint64_t value, unsigned size)
>> +{
>
> I find these read and write functions quite hard to read. I'm
> wondering if it would work better to split them up into smaller
> memory regions which get registered at the right addresses, rather
> than effectively doing the decode into different subsections based
> on address and g->part here?
>

Initially I implemented these functions splitted like you suggest, but 
that left me with three basically identical long switch statements, so I 
decided to combine them into one to achieve better code reuse. If you 
think code reuse is not an issue here, I'll gladly return it back to 
splitted implementation.

>> +    Exynos4GpioState *g = (Exynos4GpioState *)opaque;
>> +    unsigned int port_end, extint_con_start, extint_con_end;
>> +    unsigned int extint_flt_start, extint_flt_end;
>> +    unsigned int extint_mask_start, extint_mask_end;
>> +    unsigned int extint_pend_start, extint_pend_end;
>> +    unsigned int etcp_start_addr, etcp_start_idx, extint_pri_end;
>> +    unsigned idx;
>> +
>> +    DPRINT_L2("GPIO%u write off 0x%x = %u(0x%x)\n", g->part,
>> +            (uint32_t)off, (uint32_t)value, (uint32_t)value);
>> +
>> +    switch (g->part) {
>> +    case GPIO_PART2X:
>> +        port_end = GPIO2_XPORT_END;
>> +        extint_con_start = GPIO2_WKPINT_CON_START;
>> +        extint_con_end = GPIO2_WKPINT_CON_END;
>> +        extint_flt_start = GPIO2_WKPINT_FLT_START;
>> +        extint_flt_end = GPIO2_WKPINT_FLT_END;
>> +        extint_mask_start = GPIO2_WKPINT_MASK_START;
>> +        extint_mask_end = GPIO2_WKPINT_MASK_END;
>> +        extint_pend_start = GPIO2_WKPINT_PEND_START;
>> +        extint_pend_end = GPIO2_WKPINT_PEND_END;
>> +        etcp_start_addr = etcp_start_idx = extint_pri_end = 0;
>> +        break;
>> +    case GPIO_PART1: default:
>> +        port_end = GPIO_NORM_PORT_END;
>> +        extint_con_start = GPIO_EXTINT_CON_START;
>> +        extint_con_end = GPIO1_EXTINT_CON_END;
>> +        extint_flt_start = GPIO_EXTINT_FLT_START;
>> +        extint_flt_end = GPIO1_EXTINT_FLT_END;
>> +        extint_mask_start = GPIO_EXTINT_MASK_START;
>> +        extint_mask_end = GPIO1_EXTINT_MASK_END;
>> +        extint_pend_start = GPIO_EXTINT_PEND_START;
>> +        extint_pend_end = GPIO1_EXTINT_PEND_END;
>> +        etcp_start_addr = GPIO1_ETCPORT_START;
>> +        etcp_start_idx = GPIO1_NORM_PORT_NUM;
>> +        extint_pri_end = GPIO1_EXTINT_FIXPRI_END;
>> +        break;
>> +    case GPIO_PART2:
>> +        port_end = GPIO_NORM_PORT_END;
>> +        extint_con_start = GPIO_EXTINT_CON_START;
>> +        extint_con_end = GPIO2_EXTINT_CON_END;
>> +        extint_flt_start = GPIO_EXTINT_FLT_START;
>> +        extint_flt_end = GPIO2_EXTINT_FLT_END;
>> +        extint_mask_start = GPIO_EXTINT_MASK_START;
>> +        extint_mask_end = GPIO2_EXTINT_MASK_END;
>> +        extint_pend_start = GPIO_EXTINT_PEND_START;
>> +        extint_pend_end = GPIO2_EXTINT_PEND_END;
>> +        etcp_start_addr = GPIO2_ETCPORT_START;
>> +        etcp_start_idx = GPIO2_NORM_PORT_NUM;
>> +        extint_pri_end = GPIO2_EXTINT_FIXPRI_END;
>> +        break;
>> +    case GPIO_PART3:
>> +        if (off<  GPIO3_NORM_PORT_END) {
>> +            idx = DIV_BY_PORTGR_SIZE(off);
>> +            exynos4_gpio_portgr_write(g, idx, MOD_PORTGR_SIZE(off), value);
>> +        } else {
>> +            DPRINT_ERROR("GPIO3 bad write off 0x%x = %u(0x%x)\n",
>> +                    (uint32_t)off, (uint32_t)value, (uint32_t)value);
>> +        }
>> +        return;
>> +    }
>> +
>> +    if (off<  port_end) {
>> +        idx = DIV_BY_PORTGR_SIZE(off);
>> +        exynos4_gpio_portgr_write(g, idx, MOD_PORTGR_SIZE(off), value);
>> +    } else if (off>= extint_mask_start&&  off<  extint_mask_end) {
>> +        idx = (off - extint_mask_start)>>  2;
>> +        DPRINT_L1("GPIO%u EXTINT%u_MASK register write = %u(0x%x)\n", g->part,
>> +            g->port_ints[idx].int_line_num, (uint32_t)value, (uint32_t)value);
>> +        g->port_ints[idx].mask = value;
>> +    } else if (off>= extint_pend_start&&  off<  extint_pend_end) {
>> +        idx = (off - extint_pend_start)>>  2;
>> +        DPRINT_L1("GPIO%u EXTINT%u_PEND register write = %u(0x%x)\n", g->part,
>> +            g->port_ints[idx].int_line_num, (uint32_t)value, (uint32_t)value);
>> +        if (g->part == GPIO_PART2X) {
>> +            unsigned i, irq_n;
>> +            Exynos4Gpio2XState *g2 = EXYNOS4210_GPIO2X(g);
>> +            for (i = 0; i<  GPIO_MAX_PIN_IN_PORT; i++) {
>> +                if ((g->port_ints[idx].pend&  (1<<  i))&&  (value&  (1<<  i))) {
>> +                    g->port_ints[idx].pend&= ~(1<<  i);
>> +                    irq_n = idx * GPIO_MAX_PIN_IN_PORT + i;
>> +                    if (irq_n>= GPIO2_X_PORT_IRQ_NUM) {
>> +                        irq_n = GPIO2_X_PORT_IRQ_NUM - 1;
>> +                    }
>> +                    qemu_irq_lower(g2->ext_irq[irq_n]);
>> +                }
>> +            }
>> +        } else {
>> +            g->port_ints[idx].pend&= ~value;
>> +            exynos4_gpioirq_update(g);
>> +        }
>> +    } else if (off>= extint_con_start&&  off<  extint_con_end) {
>> +        idx = (off - extint_con_start)>>  2;
>> +        DPRINT_L1("GPIO%u EXTINT%u_CON register write = %u(0x%x)\n", g->part,
>> +            g->port_ints[idx].int_line_num, (uint32_t)value, (uint32_t)value);
>> +        g->port_ints[idx].con = value;
>> +    } else if (off>= extint_flt_start&&  off<  extint_flt_end) {
>> +        unsigned i = ((off - extint_flt_start)>>  2)&  1;
>> +        idx = (off - extint_flt_start)>>  3;
>> +        DPRINT_L1("GPIO%u EXTINT%u_FLTCON%u reg write = %u(0x%x)\n", g->part,
>> +          g->port_ints[idx].int_line_num, i, (uint32_t)value, (uint32_t)value);
>> +        g->port_ints[idx].fltcon[i] = value;
>> +    } else if (g->part == GPIO_PART2X) {
>> +        DPRINT_ERROR("GPIO2 group X bad write off 0x%x = %u(0x%x)\n",
>> +                (uint32_t)off, (uint32_t)value, (uint32_t)value);
>> +        return;
>> +    } else if (off == GPIO_EXTINT_SERVICE) {
>> +        DPRINT_L1("GPIO%u EXT_INT_SERVICE register write = %u(0x%x)\n",
>> +                g->part, (uint32_t)value, (uint32_t)value);
>> +        EXYNOS4210_GPIO_1_2(g)->extint_serv = value;
>> +    } else if (off == GPIO_EXTINT_SERVICE_PEND) {
>> +        DPRINT_L1("GPIO%u EXT_INT_SERVICE_PEND register write = %u(0x%x)\n",
>> +                g->part, (uint32_t)value, (uint32_t)value);
>> +        EXYNOS4210_GPIO_1_2(g)->extint_serv_pend = value;
>> +    } else if (off == GPIO_EXTINT_GRPFIXPRI) {
>> +        DPRINT_L1("GPIO%u EXT_INT_GRPFIXPRI register write = %u(0x%x)\n",
>> +                g->part, (uint32_t)value, (uint32_t)value);
>> +        EXYNOS4210_GPIO_1_2(g)->extint_grpfixpri = value;
>> +    } else if (off>= GPIO_EXTINT_FIXPRI_START&&  off<  extint_pri_end) {
>> +        idx = (off - GPIO_EXTINT_FIXPRI_START)>>  2;
>> +        DPRINT_L1("GPIO%u EXTINT%u_FIXPRI register write = %u(0x%x)\n", g->part,
>> +            g->port_ints[idx].int_line_num, (uint32_t)value, (uint32_t)value);
>> +        g->port_ints[idx].fixpri = value;
>> +    } else if (off>= etcp_start_addr&&  off<  GPIO_ETCPORT_END) {
>> +        idx = etcp_start_idx + DIV_BY_PORTGR_SIZE(off - etcp_start_addr);
>> +        exynos4_etc_portgroup_write(&g->ports[idx],
>> +            MOD_PORTGR_SIZE(off - etcp_start_addr), value);
>> +    } else {
>> +        DPRINT_ERROR("GPIO%u bad write off 0x%x = %u(0x%x)\n", g->part,
>> +            (uint32_t)off, (uint32_t)value, (uint32_t)value);
>> +    }
>> +}
>> +static void exynos4_gpio1_init(Object *obj)
>> +{
>> +    Exynos4GpioState *g = EXYNOS4210_GPIO(obj);
>> +
>> +    g->part = GPIO_PART1;
>> +    g->ports = gpio1_ports;
>> +    g->port_ints = gpio1_ports_interrupts;
>> +    g->num_of_ports = sizeof(gpio1_ports) / sizeof(Exynos4PortGroup);
>> +    g->num_of_portints =
>> +            sizeof(gpio1_ports_interrupts) / sizeof(Exynos4PortIntState);
>
> You can use the ARRAY_SIZE macro here.
>
> -- PMM
>
>


-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com