Re: [PATCH v35 10/13] target/avr: Add limited support for USART and 16 bit timer peripherals

[Philippe Mathieu-Daudé]

On 11/22/19 3:41 PM, Aleksandar Markovic wrote:
> On Tue, Oct 29, 2019 at 10:25 PM Michael Rolnik <address@hidden> wrote:
> > From: Sarah Harris <address@hidden>
> >
> > These were designed to facilitate testing but should provide enough function to 
> > be useful in other contexts.
> > Only a subset of the functions of each peripheral is implemented, mainly due to 
> > the lack of a standard way to handle electrical connections (like GPIO pins).
> >
> > Signed-off-by: Sarah Harris <address@hidden>
> > ---
> >   hw/char/Kconfig                |   3 +
> >   hw/char/Makefile.objs          |   1 +
> >   hw/char/avr_usart.c            | 324 ++++++++++++++++++
> >   hw/misc/Kconfig                |   3 +
> >   hw/misc/Makefile.objs          |   2 +
> >   hw/misc/avr_mask.c             | 112 ++++++
> >   hw/timer/Kconfig               |   3 +
> >   hw/timer/Makefile.objs         |   2 +
> >   hw/timer/avr_timer16.c         | 605 +++++++++++++++++++++++++++++++++
> >   include/hw/char/avr_usart.h    |  97 ++++++
> >   include/hw/misc/avr_mask.h     |  47 +++
> >   include/hw/timer/avr_timer16.h |  97 ++++++
> >   12 files changed, 1296 insertions(+)
> >   create mode 100644 hw/char/avr_usart.c
> >   create mode 100644 hw/misc/avr_mask.c
> >   create mode 100644 hw/timer/avr_timer16.c
> >   create mode 100644 include/hw/char/avr_usart.h
> >   create mode 100644 include/hw/misc/avr_mask.h
> >   create mode 100644 include/hw/timer/avr_timer16.h
> >
> > diff --git a/hw/char/Kconfig b/hw/char/Kconfig
> > index 40e7a8b8bb..331b20983f 100644
> > --- a/hw/char/Kconfig
> > +++ b/hw/char/Kconfig
> > @@ -46,3 +46,6 @@ config SCLPCONSOLE
> >
> >   config TERMINAL3270
> >       bool
> > +
> > +config AVR_USART
> > +    bool
> > diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> > index 02d8a66925..f05c1f5667 100644
> > --- a/hw/char/Makefile.objs
> > +++ b/hw/char/Makefile.objs
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_PSERIES) += spapr_vty.o
> >   obj-$(CONFIG_DIGIC) += digic-uart.o
> >   obj-$(CONFIG_STM32F2XX_USART) += stm32f2xx_usart.o
> >   obj-$(CONFIG_RASPI) += bcm2835_aux.o
> > +common-obj-$(CONFIG_AVR_USART) += avr_usart.o
> >
> >   common-obj-$(CONFIG_CMSDK_APB_UART) += cmsdk-apb-uart.o
> >   common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
> > diff --git a/hw/char/avr_usart.c b/hw/char/avr_usart.c
> > new file mode 100644
> > index 0000000000..9ca3c2a1cd
> > --- /dev/null
> > +++ b/hw/char/avr_usart.c
> > @@ -0,0 +1,324 @@
> > +/*
> > + * AVR USART
> > + *
> > + * Copyright (c) 2018 University of Kent
> > + * Author: Sarah Harris
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to 
> > deal
> > + * in the Software without restriction, including without limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> > FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/char/avr_usart.h"
> > +#include "qemu/log.h"
> > +#include "hw/irq.h"
> > +#include "hw/qdev-properties.h"
> > +
> > +static int avr_usart_can_receive(void *opaque)
> > +{
> > +    AVRUsartState *usart = opaque;
> > +
> > +    if (usart->data_valid || !(usart->csrb & USART_CSRB_RXEN)) {
> > +        return 0;
> > +    }
> > +    return 1;

Here we tell the chardev frontend that we can receive at most 1 byte, ...

> > +}
> > +
> > +static void avr_usart_receive(void *opaque, const uint8_t *buffer, int size)
> > +{
> > +    AVRUsartState *usart = opaque;
> > +    assert(size == 1);

... so this condition is true, the frontend will never provide us more 
than 1 byte.

> Hello, Michael.
>
> I see the line "assert(size == 1);" is used here, and in really numerous
> places in USART emulation (as a rule, at the very beginnings of function
> bodies). Could you explain to me the justification for that line? Is there
> a place in documentation that would expain the need for it? If this is
> justified, why is there the need for argument "int size" in corresponding
> functions? If some external rule/API forces you to have that argument for
> all such functions, can you tell me what rule/API is that?

Some backends have FIFO queues, so can process more chars at once.

> Yours,
> Aleksandar
>
> > +    assert(!usart->data_valid);
> > +    usart->data = buffer[0];

Here the model consumes the 1st char of an array of at most 1 byte.

I suppose Sarah wanted to be sure we are not dropping characters.

> > +    usart->data_valid = true;
> > +    usart->csra |= USART_CSRA_RXC;
> > +    if (usart->csrb & USART_CSRB_RXCIE) {
> > +        qemu_set_irq(usart->rxc_irq, 1);
> > +    }
> > +}
[...]