Re: [Qemu-devel] [PATCH v1 1/5] Netduino_USART: Add the Netduino Plus 2 USART Controller

[Alistair Francis]

On Sun, Aug 24, 2014 at 12:09 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Sun, Aug 24, 2014 at 10:13 AM, Alistair Francis <address@hidden> wrote:
>> This patch adds the Netduino Plus 2 USART controller
>> (UART also uses the same controller).
>>
>> It can be used for reading and writing to the guest.
>>
>
> Do you just mean doing IO with the system?

Sorry, that was a bit confusing. At the moment it's just with itself

>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>>  default-configs/arm-softmmu.mak |   1 +
>>  hw/char/Makefile.objs           |   1 +
>>  hw/char/netduino_usart.c        | 252 
>> ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 254 insertions(+)
>>  create mode 100644 hw/char/netduino_usart.c
>>
>> diff --git a/default-configs/arm-softmmu.mak 
>> b/default-configs/arm-softmmu.mak
>> index f3513fa..46471ad 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -78,6 +78,7 @@ CONFIG_NSERIES=y
>>  CONFIG_REALVIEW=y
>>  CONFIG_ZAURUS=y
>>  CONFIG_ZYNQ=y
>> +CONFIG_NETDUINOP2=y
>>
>>  CONFIG_VERSATILE_PCI=y
>>  CONFIG_VERSATILE_I2C=y
>> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
>> index 317385d..c03aa98 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_NETDUINOP2) += netduino_usart.o
>>
>>  common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
>>  common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
>> diff --git a/hw/char/netduino_usart.c b/hw/char/netduino_usart.c
>> new file mode 100644
>> index 0000000..437af2b
>> --- /dev/null
>> +++ b/hw/char/netduino_usart.c
>> @@ -0,0 +1,252 @@
>> +/*
>> + * Netduino Plus 2 USART
>> + *
>
> I thing the UART IP is really part of the ST Microcontroller, and
> should be named as such. Do ST give this controller a meaningful name
> in their documentation? Looking at the schematic for Netduino it looks
> like there is very little board level IP. All the sysbus IP is local
> to the microcontroller. I think the name "netduino" should probably
> only be limited to the board level.

ST don't give it a name, besides USART. I can change change the
Netduino part to 'STM32F405xx'. That probably models the documentation
the best.

>
>> + * 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 DEBUG_NETUSART */
>
> Don't worry about commented out //#defines
>
>> +
>> +#ifdef DEBUG_NETUSART
>> +#define DPRINTF(fmt, ...) \
>
> You should change this to a always-compiled print using a regular if.
> Check hw/dma/pl330.c

Yep, can do

>
>> +do { printf("netduino_usart: " fmt , ## __VA_ARGS__); } while (0)
>
> Multiple lines to keep formatting standard even within #define. use \ as 
> needed.
>
> Use qemu_log instead of printf. (pl330 fails at this too).
>

Will do

>> +#else
>> +#define DPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#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_NETDUINO_USART "netduino_usart"
>
> Change "_" to "-" in QOM typename.
>

Done

>> +#define NETDUINO_USART(obj) \
>> +    OBJECT_CHECK(struct net_usart, (obj), TYPE_NETDUINO_USART)
>> +
>> +struct net_usart {
>
> CamelCase in typename. Use a typedef to avoid the repeated struct Foo
> all throughout.
>

Done

>> +    SysBusDevice parent_obj;
>> +
>> +    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;
>> +};
>
> The struct, QOM typename and register defines should go in a device
> specific header. Check include/hw/char/digic_uart.h for an example.
>

Is that necessary for this case? All of these are only called from one place
and there aren't that many lines to be moved to a header.

>> +
>> +static int usart_can_receive(void *opaque)
>> +{
>> +    struct net_usart *s = opaque;
>> +
>> +    if (s->usart_cr1 & USART_CR1_UE && s->usart_cr1 & USART_CR1_TE) {
>> +        return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void usart_receive(void *opaque, const uint8_t *buf, int size)
>> +{
>> +    struct net_usart *s = opaque;
>> +    int i, num;
>> +
>> +    num = size > 8 ? 8 : size;
>
> MAX macro.
>

Will do

>> +    s->usart_dr = 0;
>> +
>> +    for (i = 0; i < num; i++) {
>> +        s->usart_dr |= (buf[i] << i);
>> +    }
>
> This seems strange. dr is an oring of multiple rxed characters? Is RX
> information lost here when size > 1?
>

This is really wrong, it will be accessing invalid memory if size > 1

I will fix this, I am getting too used to VHDL

>> +
>> +    s->usart_sr |= USART_SR_RXNE;
>> +
>> +    if (s->usart_cr1 & USART_CR1_RXNEIE) {
>> +        qemu_set_irq(s->irq, 1);
>> +    }
>> +
>> +    DPRINTF("Inputting: %c\n", s->usart_dr);
>
> "receiving"
>

Done

>> +}
>> +
>> +static void usart_reset(DeviceState *dev)
>> +{
>> +    struct net_usart *s = NETDUINO_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 netduino_usart_read(void *opaque, hwaddr addr,
>> +                                    unsigned int size)
>> +{
>> +    struct net_usart *s = opaque;
>> +    uint64_t retvalue;
>> +
>> +    DPRINTF("Read 0x%x\n", (uint) addr);
>> +
>> +    switch (addr) {
>> +    case USART_SR:
>> +        retvalue = s->usart_sr;
>> +        s->usart_sr &= (USART_SR_TC ^ 0xFFFF);
>> +        return retvalue;
>> +    case USART_DR:
>> +        DPRINTF("Value: %x, %c\n", s->usart_dr, s->usart_dr);
>
> PRIx32 instead of %x. Cast the %c version.
>
>> +        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;
>
> If you make your registers an array, you can index the with the offset
> and remove the repeated s->foo return logic. You can also use a single
> memset to do all the 0 resets.
>

Does that comply with QEMU's style. I haven't seen any other devices
that do that.

>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "net_usart_read: Bad offset %x\n", (int)addr);
>> +        return 0;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void netduino_usart_write(void *opaque, hwaddr addr,
>> +                       uint64_t val64, unsigned int size)
>> +{
>> +    struct net_usart *s = opaque;
>> +    uint32_t value = (uint32_t) val64;
>> +    unsigned char ch;
>> +
>> +    DPRINTF("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);
>> +            }
>> +            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,
>> +                      "net_usart_write: Bad offset %x\n", (int)addr);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps netduino_usart_ops = {
>> +    .read = netduino_usart_read,
>> +    .write = netduino_usart_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static int netduino_usart_init(SysBusDevice *sbd)
>> +{
>> +    DeviceState *dev = DEVICE(sbd);
>> +    struct net_usart *s = NETDUINO_USART(dev);
>> +
>> +    sysbus_init_irq(sbd, &s->irq);
>> +
>> +    memory_region_init_io(&s->mmio, OBJECT(s), &netduino_usart_ops, s,
>> +                          TYPE_NETDUINO_USART, 0x2000);
>> +    sysbus_init_mmio(sbd, &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);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void netduino_usart_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +
>> +    k->init = netduino_usart_init;
>
> Use of Sysbus::init is deprecated, please use device init and realize instead.
>

Yep, will fix

>> +    dc->props = NULL;
>
> Unneeded.
>
> Some comments also apply to the following patches.

Thanks, I'll make the changes

>
> Regards,
> Peter
>
>> +    dc->reset = usart_reset;
>> +}
>> +
>> +static const TypeInfo netduino_usart_info = {
>> +    .name          = TYPE_NETDUINO_USART,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(struct net_usart),
>> +    .class_init    = netduino_usart_class_init,
>> +};
>> +
>> +static void netduino_usart_register_types(void)
>> +{
>> +    type_register_static(&netduino_usart_info);
>> +}
>> +
>> +type_init(netduino_usart_register_types)
>> --
>> 1.9.1
>>
>>