Re: [Qemu-devel] [PATCH] nios2: Add Altera JTAG UART emulation

[Marek Vasut <marex@denx.de>]

On 02/12/2017 07:55 PM, Juro Bystricky wrote:

Subject should contain [PATCH v5] ...

> Add the Altera JTAG UART model.
> 
> Hardware emulation based on:
> https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
> (Please see "Register Map" on page 65)

[...]

> +static void altera_juart_write(void *opaque, hwaddr addr,
> +                       uint64_t value, unsigned int size)
> +{
> +    AlteraJUARTState *s = opaque;
> +    unsigned char c;
> +
> +    switch (addr) {
> +    case OFFSET_R_DATA:
> +        c = value & 0xFF;
> +        /*
> +         * We do not decrement the write fifo,
> +         * we "tranmsmit" instanteniously, CONTROL_WI always asserted

Nit, fix this comment (transmit, instantly) and it's alignment.

I don't quite understand what this comment is trying to tell me though.

> +         */
> +        s->jcontrol |= CONTROL_WI;
> +        s->jdata = c;
> +        qemu_chr_fe_write(&s->chr, &c, 1);
> +        altera_juart_update_irq(s);
> +        break;
> +
> +    case OFFSET_R_CONTROL:
> +        /* Only RE and WE are writable */
> +        value &= CONTROL_WMASK;
> +        s->jcontrol &= ~CONTROL_WMASK;
> +        s->jcontrol |= value;
> +
> +        /* Writing 1 to AC clears it to 0 */
> +        if (value & CONTROL_AC) {
> +            s->jcontrol &= ~CONTROL_AC;
> +        }
> +        altera_juart_update_irq(s);
> +        break;
> +    }
> +}

[...]

> +void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq, uint32_t fifo_size)
> +{
> +    DeviceState *dev;
> +    SysBusDevice *bus;
> +    Chardev *chr;
> +    const char chr_name[] = "juart";
> +    char label[ARRAY_SIZE(chr_name) + 1];
> +
> +    dev = qdev_create(NULL, TYPE_ALTERA_JUART);
> +
> +    if (channel >= MAX_SERIAL_PORTS) {
> +        error_report("Only %d serial ports are supported by QEMU",
> +                     MAX_SERIAL_PORTS);
> +        exit(1);
> +    }
> +
> +    chr = serial_hds[channel];
> +    if (!chr) {
> +        snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, channel);
> +        chr = qemu_chr_new(label, "null");
> +        if (!chr) {
> +            error_report("Failed to assign serial port to altera %s", label);
> +            exit(1);
> +        }
> +    }
> +    qdev_prop_set_chr(dev, "chardev", chr);
> +
> +    /*
> +     * FIFO size can be set from 8 to 32,768 bytes.
> +     * Only powers of two are allowed.
> +     */
> +    if (fifo_size < 8 || fifo_size > 3276 || (fifo_size & ~(1 << ctz32(fifo_size)))) {

32768 , not 3276 ...

> +        error_report("juart%d: Invalid FIFO size. [%u]", channel, fifo_size);
> +        exit(1);
> +    }
> +
> +    qdev_prop_set_uint32(dev, "fifo-size", fifo_size);
> +    bus = SYS_BUS_DEVICE(dev);
> +    qdev_init_nofail(dev);
> +
> +    if (addr != (hwaddr)-1) {
> +        sysbus_mmio_map(bus, 0, addr);
> +    }
> +
> +    sysbus_connect_irq(bus, 0, irq);
> +}

[...]

> +typedef struct AlteraJUARTState {
> +    SysBusDevice busdev;
> +    MemoryRegion mmio;
> +    CharBackend chr;
> +    qemu_irq irq;
> +
> +    unsigned int rx_fifo_size;
> +    unsigned int rx_fifo_pos;
> +    unsigned int rx_fifo_len;
> +    uint32_t jdata;
> +    uint32_t jcontrol;
> +    uint8_t *rx_fifo;
> +} AlteraJUARTState;
> +
> +void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq,
> +                                                        uint32_t fifo_size);

Fix the alignment here so it doesn't look so braindead, align under the
first open parenthesis.

> +#endif /* ALTERA_JUART_H */
> 


-- 
Best regards,
Marek Vasut