Re: [Qemu-devel] [RFC v1 2/6] stm32f205_USART: Add the stm32f205 SoC USART Controller

[Peter Crosthwaite]

On Tue, Sep 9, 2014 at 6:24 PM, Alistair Francis <address@hidden> wrote:
> This patch adds the stm32f205 USART controller
> (UART also uses the same controller).
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> V2:
>  - Small changes thanks to Peter C
>  - Rename for the Netduino 2 and it's SoC
>
>  hw/char/Makefile.objs             |   1 +
>  hw/char/stm32f205_usart.c         | 205 
> ++++++++++++++++++++++++++++++++++++++
>  include/hw/char/stm32f205_usart.h |  64 ++++++++++++
>  3 files changed, 270 insertions(+)
>  create mode 100644 hw/char/stm32f205_usart.c
>  create mode 100644 include/hw/char/stm32f205_usart.h
>
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 317385d..b1f7e80 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -15,6 +15,7 @@ obj-$(CONFIG_OMAP) += omap_uart.o
>  obj-$(CONFIG_SH4) += sh_serial.o
>  obj-$(CONFIG_PSERIES) += spapr_vty.o
>  obj-$(CONFIG_DIGIC) += digic-uart.o
> +obj-$(CONFIG_NETDUINO2) += stm32f205_usart.o
>
>  common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
>  common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
> diff --git a/hw/char/stm32f205_usart.c b/hw/char/stm32f205_usart.c
> new file mode 100644
> index 0000000..c042b4b
> --- /dev/null
> +++ b/hw/char/stm32f205_usart.c
> @@ -0,0 +1,205 @@
> +/*
> + * STM32F205xx USART
> + *
> + * Copyright (c) 2014 Alistair Francis <address@hidden>
> + *
> + * 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 "hw/char/stm32f205_usart.h"
> +
> +#ifndef STM_USART_ERR_DEBUG
> +#define STM_USART_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT_L(lvl, fmt, args...) do { \
> +    if (STM_USART_ERR_DEBUG >= lvl) { \
> +        fprintf(stderr, "stm32f205xx_usart: %s:" fmt, __func__, ## args); \
> +    } \
> +} while (0);
> +
> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
> +
> +static int usart_can_receive(void *opaque)
> +{
> +    Stm32f205UsartState *s = opaque;
> +
> +    if (s->usart_cr1 & USART_CR1_UE && s->usart_cr1 & USART_CR1_TE) {
> +        return 1;
> +    }

So it's usual to block a UART on the fifo filling rather than the
master enable switches. Corking the fifo on the master enable means
QEMU will buffer UART input long-term until the guest turns the fifo
on, where in reality the hardware should just drop the chars. We
should do something similar.

The reason (as far as I know anyways) for can_recieve and giving qemu
serial false closed-loop implementation (network has it too) is to
deal with large amounts of instantaneous data in situations where the
real-time delays on real-hardware would give natural bandwidth
limiting. Thus the main app of can-recieve 0-return is 'your pumping
serial data way too fast such that my fifo is full and real hardware
would never go that fast'.

Also, did you me the RE bit rather than the TE bit?

> +
> +    return 0;
> +}
> +
> +static void usart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +    Stm32f205UsartState *s = opaque;
> +
> +    s->usart_dr = *buf;
> +
> +    s->usart_sr |= USART_SR_RXNE;
> +

This might be a good condition to block can_recieve on - the
USART_SR_RXNE. And if the enables are off, just drop the char.

> +    if (s->usart_cr1 & USART_CR1_RXNEIE) {
> +        qemu_set_irq(s->irq, 1);
> +    }
> +
> +    DB_PRINT("Receiving: %c\n", s->usart_dr);
> +}
> +
> +static void usart_reset(DeviceState *dev)

stm32f..._ prefix to function name. For consistency and makes gdb
breakpoints slightly easier if function names are globally unique.

> +{
> +    Stm32f205UsartState *s = STM32F205xx_USART(dev);
> +
> +    s->usart_sr = 0x00C00000;
> +    s->usart_dr = 0x00000000;
> +    s->usart_brr = 0x00000000;
> +    s->usart_cr1 = 0x00000000;
> +    s->usart_cr2 = 0x00000000;
> +    s->usart_cr3 = 0x00000000;
> +    s->usart_gtpr = 0x00000000;
> +}
> +
> +static uint64_t stm32f205xx_usart_read(void *opaque, hwaddr addr,
> +                                    unsigned int size)
> +{
> +    Stm32f205UsartState *s = opaque;
> +    uint64_t retvalue;
> +
> +    DB_PRINT("Read 0x%x\n", (uint) addr);
> +
> +    switch (addr) {
> +    case USART_SR:
> +        retvalue = s->usart_sr;
> +        s->usart_sr &= (USART_SR_TC ^ 0xFFFF);

This register seems to be a mix of clear-to-read, read-only and rsvd.
Just use &= ~MASK to implement clear on read which look to be the
intention here. I don't see the significance of bits 15 downto 0 and
why they behave different to 31 downto 16 here?

This is probably also where you need to repoll for can_receive blocked
chars as well (assuming you make condition change I recommended above.

 qemu_chr_accept_input(s->chr)

> +        return retvalue;
> +    case USART_DR:
> +        DB_PRINT("Value: 0x%x, %c\n", s->usart_dr, (char) s->usart_dr);
> +        s->usart_sr |= USART_SR_TXE;
> +        return s->usart_dr & 0x3FF;
> +    case USART_BRR:
> +        return s->usart_brr;
> +    case USART_CR1:
> +        return s->usart_cr1;
> +    case USART_CR2:
> +        return s->usart_cr2;
> +    case USART_CR3:
> +        return s->usart_cr3;
> +    case USART_GTPR:
> +        return s->usart_gtpr;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "STM32F205xx_usart_read: Bad offset %x\n", (int)addr);
> +        return 0;
> +    }
> +
> +    return 0;
> +}
> +
> +static void stm32f205xx_usart_write(void *opaque, hwaddr addr,
> +                       uint64_t val64, unsigned int size)
> +{
> +    Stm32f205UsartState *s = opaque;
> +    uint32_t value = (uint32_t) val64;
> +    unsigned char ch;
> +
> +    DB_PRINT("Write 0x%x, 0x%x\n", value, (uint) addr);
> +
> +    switch (addr) {
> +    case USART_SR:
> +        if (value <= 0x3FF) {
> +            s->usart_sr = value;
> +        } else {
> +            s->usart_sr &= value;
> +        }
> +        return;
> +    case USART_DR:
> +        if (value < 0xF000) {
> +            ch = value;
> +            if (s->chr) {
> +                qemu_chr_fe_write(s->chr, &ch, 1);

If the backend gets busy this will drop chars. You can either
implement the closed loop callbacks (kinda hard - check serial.c) or
use qemu_chr_fe_write all to get a blocking implementation.

> +            }
> +            s->usart_sr |= USART_SR_TC;
> +        }
> +        return;
> +    case USART_BRR:
> +        s->usart_brr = value;
> +        return;
> +    case USART_CR1:
> +        s->usart_cr1 = value;
> +        return;
> +    case USART_CR2:
> +        s->usart_cr2 = value;
> +        return;
> +    case USART_CR3:
> +        s->usart_cr3 = value;
> +        return;
> +    case USART_GTPR:
> +        s->usart_gtpr = value;
> +        return;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "STM32F205xx_usart_write: Bad offset %x\n", (int)addr);
> +    }
> +}
> +
> +static const MemoryRegionOps stm32f205xx_usart_ops = {
> +    .read = stm32f205xx_usart_read,
> +    .write = stm32f205xx_usart_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void stm32f205xx_usart_init(Object *obj)
> +{
> +    Stm32f205UsartState *s = STM32F205xx_USART(obj);
> +
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
> +
> +    memory_region_init_io(&s->mmio, obj, &stm32f205xx_usart_ops, s,
> +                          TYPE_STM32F205_USART, 0x2000);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +
> +    s->chr = qemu_char_get_next_serial();
> +
> +    if (s->chr) {
> +        qemu_chr_add_handlers(s->chr, usart_can_receive, usart_receive,
> +                              NULL, s);
> +    }
> +}
> +
> +static void stm32f205xx_usart_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = usart_reset;
> +}
> +
> +static const TypeInfo stm32f205xx_usart_info = {
> +    .name          = TYPE_STM32F205_USART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(Stm32f205UsartState),
> +    .instance_init = stm32f205xx_usart_init,
> +    .class_init    = stm32f205xx_usart_class_init,
> +};
> +
> +static void stm32f205xx_usart_register_types(void)
> +{
> +    type_register_static(&stm32f205xx_usart_info);
> +}
> +
> +type_init(stm32f205xx_usart_register_types)
> diff --git a/include/hw/char/stm32f205_usart.h 
> b/include/hw/char/stm32f205_usart.h
> new file mode 100644
> index 0000000..ceb92b7
> --- /dev/null
> +++ b/include/hw/char/stm32f205_usart.h
> @@ -0,0 +1,64 @@
> +/*
> + * STM32F205xx USART
> + *
> + * Copyright (c) 2014 Alistair Francis <address@hidden>
> + *
> + * 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 "hw/sysbus.h"
> +#include "sysemu/char.h"
> +#include "hw/hw.h"
> +
> +#define USART_SR   0x00
> +#define USART_DR   0x04
> +#define USART_BRR  0x08
> +#define USART_CR1  0x0C
> +#define USART_CR2  0x10
> +#define USART_CR3  0x14
> +#define USART_GTPR 0x18
> +
> +#define USART_SR_TXE  (1 << 7)
> +#define USART_SR_TC   (1 << 6)
> +#define USART_SR_RXNE (1 << 5)
> +
> +#define USART_CR1_UE  (1 << 13)
> +#define USART_CR1_RXNEIE  (1 << 5)
> +#define USART_CR1_TE  (1 << 3)
> +
> +#define TYPE_STM32F205_USART "stm32f205xx-usart"
> +#define STM32F205xx_USART(obj) \
> +    OBJECT_CHECK(Stm32f205UsartState, (obj), TYPE_STM32F205_USART)
> +
> +typedef struct {

/*< private >*/

> +    SysBusDevice parent_obj;
> +

/*< public >*/

> +    MemoryRegion mmio;
> +
> +    uint32_t usart_sr;
> +    uint32_t usart_dr;
> +    uint32_t usart_brr;
> +    uint32_t usart_cr1;
> +    uint32_t usart_cr2;
> +    uint32_t usart_cr3;
> +    uint32_t usart_gtpr;
> +
> +    CharDriverState *chr;
> +    qemu_irq irq;
> +} Stm32f205UsartState;

STM

A few general styling comments from prev patch (mainly a few casts)
apply here as well.

Regards,
Peter

> --
> 1.9.1
>
>