[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH v2 2/9] hw/char/cmsdk-apb-uart.c: Implement CMSDK
From: |
Alex Bennée |
Subject: |
Re: [Qemu-arm] [PATCH v2 2/9] hw/char/cmsdk-apb-uart.c: Implement CMSDK APB UART |
Date: |
Fri, 14 Jul 2017 17:11:41 +0100 |
User-agent: |
mu4e 0.9.19; emacs 25.2.50.3 |
Peter Maydell <address@hidden> writes:
> 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.
Sure.
Reviewed-by: Alex Bennée <address@hidden>
>
> thanks
> -- PMM
--
Alex Bennée
[Qemu-arm] [PATCH v2 1/9] hw/arm/mps2: Implement skeleton mps2-an385 and mps2-an511 board models, Peter Maydell, 2017/07/14
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 0/9] ARM: implement MPS2 board (with 2 FPGA flavours), no-reply, 2017/07/14
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 0/9] ARM: implement MPS2 board (with 2 FPGA flavours), no-reply, 2017/07/14