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

[Alistair Francis]

On Tue, Jul 11, 2017 at 6:45 PM, Peter Maydell <address@hidden> wrote:
> On 11 July 2017 at 16:40, Alistair Francis <address@hidden> wrote:
>> On Tue, Jul 11, 2017 at 5:33 PM, Peter Maydell <address@hidden> wrote:
>>> On 11 July 2017 at 16:12, Alistair Francis <address@hidden> wrote:
>>>> On Tue, Jul 11, 2017 at 1:17 PM, Peter Maydell <address@hidden> wrote:
>>>>> Implement a model of the simple "APB UART" provided in
>>>>> the Cortex-M System Design Kit (CMSDK).
>>>>>
>>>>> Signed-off-by: Peter Maydell <address@hidden>
>>>
>>>>> +} 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.

I think it makes sense to add. It's one line of code and can possibly
be very helpful for debugging.

>
>>>>> +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.)

I like the comment, it makes sure it is really clear.

Thanks,
Alistair

>
> thanks
> -- PMM