Re: [Qemu-devel] [Qemu-arm] [PATCH v2 2/9] hw/char/cmsdk-apb-uart.c: Implement CMSDK APB UART

[Peter Maydell]

On 14 July 2017 at 16:32, Alex Bennée <address@hidden> wrote:
>
> Peter Maydell <address@hidden> writes:
>
>> Implement a model of the simple "APB UART" provided in
>> the Cortex-M System Design Kit (CMSDK).
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>
> This fails to compile, I think since
> 81517ba37a6cec59f92396b4722861868eb0a500 change the API for
> qemu_chr_fe_set_handlers.

>> +static void cmsdk_apb_uart_update(CMSDKAPBUART *s)
>> +{
>> +    /* update outbound irqs, including handling the way the rxo and txo
>> +     * interrupt status bits are just logical AND of the overrun bit in
>> +     * STATE and the overrun interrupt enable bit in CTRL.
>> +     */
>> +    uint32_t omask = (R_INTSTATUS_RXO_MASK | R_INTSTATUS_TXO_MASK);
>> +    s->intstatus &= ~omask;
>> +    s->intstatus |= (s->state & (s->ctrl >> 2) & omask);
>> +
>> +    qemu_set_irq(s->txint, !!(s->intstatus & R_INTSTATUS_TX_MASK));
>> +    qemu_set_irq(s->rxint, !!(s->intstatus & R_INTSTATUS_RX_MASK));
>> +    qemu_set_irq(s->txovrint, !!(s->intstatus & R_INTSTATUS_TXO_MASK));
>> +    qemu_set_irq(s->rxovrint, !!(s->intstatus & R_INTSTATUS_RXO_MASK));
>> +    qemu_set_irq(s->uartint, !!(s->intstatus));
>
> If we updated qemu_set_irq to take a bool instead of int would the !!
> hack no longer be needed?

I am fairly sure we have a few places that utilize the ability
to send an integer down the qemu_irq channel.

>> +    case A_STATE:
>> +        /* Bits 0 and 1 are read only; bits 2 and 3 are W1C */
>> +        s->state &= ~(value & 0xc);
>
> I guess:
>           s->state &= ~(value & (R_STATE_TXOVERRUN_MASK | 
> R_STATE_RXOVERRUN_MASK));
>
> maybe a little too verbose.

Well, it probably ought to use the bit mask names, yes.

>> +static void cmsdk_apb_uart_realize(DeviceState *dev, Error **errp)
>> +{
>> +    CMSDKAPBUART *s = CMSDK_APB_UART(dev);
>> +
>> +    if (s->pclk_frq == 0) {
>> +        error_setg(errp, "CMSDK APB UART: pclk-frq property must be set");
>> +        return;
>> +    }
>> +
>> +    /* This UART has no flow control, so we do not need to register
>> +     * an event handler to deal with CHR_EVENT_BREAK.
>> +     */
>> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
>> +                             NULL, s, NULL, true);
>
> I think this is now:
>
>     qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
>                              NULL, NULL, s, NULL, true);

Yes. It looks like we don't have to support backend swaps
(only hw/char/serial.c and virtio-console have support for that),
so just the extra NULL argument will do fine.

I propose to just fix both these nits in target-arm rather
than resend.

thanks
-- PMM