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

[Peter Maydell <peter.maydell@linaro.org>]

On 11 July 2017 at 16:40, Alistair Francis <alistair23@gmail.com> wrote:
> On Tue, Jul 11, 2017 at 5:33 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 11 July 2017 at 16:12, Alistair Francis <alistair23@gmail.com> wrote:
>>> On Tue, Jul 11, 2017 at 1:17 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> Implement a model of the simple "APB UART" provided in
>>>> the Cortex-M System Design Kit (CMSDK).
>>>>
>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>>>> +} CMSDKAPBUART;
>>>
>>> This should be CamelCase.
>>
>> Yes, but CamelCase where all the words are all-uppercase
>> (as with CMSDK, APB and UART) is indistinguishable from
>> all-uppercase. (Compare the way we say "PCIIORegion" rather
>> than "PciIoRegion".)
>
> Good point, I feel like it just looks strange being all caps though.

Me too, but I didn't have any better naming ideas.

>>>> +static void uart_update_parameters(CMSDKAPBUART *s)
>>>> +{
>>>> +    QEMUSerialSetParams ssp;
>>>> +
>>>> +    /* This UART is always 8N1 but the baud rate is programmable.
>>>> +     * The minimum permitted bauddiv setting is 16, so we just ignore
>>>> +     * settings below that (usually this means the device has just
>>>> +     * been reset and not yet programmed).
>>>> +     */
>>>> +    if (s->bauddiv < 16 || s->bauddiv > s->pclk_frq) {
>>>> +        return;
>>>> +    }
>>>
>>> This seems like it should deserve a guest error print.
>>
>> As the comment says, that would cause us to print a spurious
>> warning every time the device was reset.
>
> I just see this being hard to debug. What about if not printing if
> baud rate is 0 but guest error otherwise?
>
> Or can this not be called from reset?

The thing is that the guest error really here is "enabled TX
but didn't program the baud rate". So if you want to flag it
up you'd want to do it in uart_write() at the point where
CTRL.TX_EN is written to 1, not in this function. I decided I
didn't really care enough to check and report that (we often
don't bother to track every possible guest mistake). It won't
make any difference to QEMU anyway except in the vanishingly
rare case that the user connects QEMU up to a real serial port.
I very nearly didn't bother to implement the set-params logic
at all (we don't have it on the pl011 and nobody's ever complained).

I can add the qemu_log on "TX_EN written as 1 and baud_div out
of range" if you think it's helpful though.

>>>> +static int uart_can_receive(void *opaque)
>>>> +{
>>>> +    CMSDKAPBUART *s = CMSDK_APB_UART(opaque);
>>>> +
>>>> +    /* We can take a char if RX is enabled and the buffer is empty */
>>>> +    if (s->ctrl & R_CTRL_RX_EN_MASK && !(s->state & R_STATE_RXFULL_MASK)) {
>>>> +        return 1;
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
>>>> +{
>>>> +    CMSDKAPBUART *s = CMSDK_APB_UART(opaque);
>>>> +
>>>> +    trace_cmsdk_apb_uart_receive(*buf);
>>>> +
>>>> +    if (!(s->ctrl & R_CTRL_RX_EN_MASK)) {
>>>> +        /* Just drop the character on the floor */
>>>> +        return;
>>>
>>> Doesn't this also deserve a guest error print.
>>
>> It's a can't-happen case because uart_can_receive() won't
>> return 1 if the EN bit is clear. Checking again here is
>> perhaps unnecessary paranoia, but it's not a guest error.
>
> Ah good point. Maybe that is worth stating?

Mmm, something like:
    /* In fact uart_can_receive() ensures that we can't be
     * called unless RX is enabled and the buffer is empty,
     * but we include this logic as documentation of what the
     * hardware does if a character arrives in these circumstances.
     */

(Or we could delete this and the RXFULL check below it
entirely.)

thanks
-- PMM