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

[Bystricky, Juro]

> Please make the commit message a bit more verbose, looking just at the
> commit message, I have no clue what patch I'm looking at. The embedded
> IP UG also has like 30 cores in it, so some detail on which core and in
> which chapter/page would help.

OK, will do

> > + * The Altera JTAG UART hardware registers are described in:
> > + * https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
> > + *
> 
> Drop the trailing newline
> 

OK, will do

> > +static uint64_t altera_juart_read(void *opaque, hwaddr addr, unsigned
> int size)
> > +{
> > +    AlteraJUARTState *s = opaque;
> > +    uint32_t r;
> > +
> > +    addr >>= 2;
> 
> Hmmmmm, how will unaligned read from one of these registers be handled
> on real HW ? ie. read from address 0x3 ? What about writes ?
> 

there is no reading/writing going on via "addr". 
This just maps the hw address into register number, where registers are at
4 bytes boundaries (so they are aligned as needed) but indexed as 1,2,3.... 
(Pretty common code in other drivers.)
But will redo the code anyway so there are no shifts.


> > +static void altera_juart_write(void *opaque, hwaddr addr,
> > +                       uint64_t val64, unsigned int size)
> > +{
> > +    AlteraJUARTState *s = opaque;
> > +    uint32_t value = val64;
> 
> Is this needed ?

Good question. Legacy code, probably not needed anymore. Same in
extraxfs_ser.c, Xilinx_uartlite.c, stm32f2xx_usart.c.
I suspect in the past some compiler generated warnings about type mismatch 
(32/64). Will remove.

> 
> > +    unsigned char c;
> > +
> > +    addr >>= 2;
> > +
> > +    switch (addr) {
> > +    case R_DATA:
> > +        c = value & 0xFF;
> > +        /*
> > +         * We do not decrement the write fifo,
> > +         * we "tranmsmit" instanteniously, CONTROL_WI always asserted
> > +         */
> > +        s->jcontrol |= CONTROL_WI;
> > +        s->jdata = c;
> > +        qemu_chr_fe_write(&s->chr, &c, 1);
> > +        altera_juart_update_irq(s);
> > +        break;
> > +
> > +    case R_CONTROL:
> > +        /* Only RE and WE are writable */
> > +        value &= CONTROL_WMASK;
> > +        s->jcontrol &= ~(CONTROL_WMASK);
> 
> The parenthesis are not needed.

OK, will remove them

> 
> > +static void altera_juart_receive(void *opaque, const uint8_t *buf, int
> size)
> > +{
> > +    int i;
> > +    AlteraJUARTState *s = opaque;
> > +
> > +    if (s->rx_fifo_len >= FIFO_LENGTH) {
> > +        fprintf(stderr, "WARNING: UART dropped char.\n");
> 
> Can this ever happen ? can_receive will IMO prevent this case.
> 

Fair question. I did not go through all QEMU code to see when/where
in the bowels of QEMU can_receive is actually called. If it is always called 
prior "receive", then this check may be redundant. Again, there are
other serial drivers that do this check as well (etraxfs, Xilinx,...)
But most don't, so I'll follow the majority and remove it.

> > diff --git a/include/hw/char/altera_juart.h
> b/include/hw/char/altera_juart.h
> > new file mode 100644
> > index 0000000..8a9c892
> > --- /dev/null
> > +++ b/include/hw/char/altera_juart.h
> > @@ -0,0 +1,44 @@
> > +/*
> > + * Altera JTAG UART emulation
> > + *
> > + * Copyright (c) 2016-2017 Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + *
> > + * You should have received a copy of the GNU General Public License
> along
> > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef ALTERA_JUART_H
> > +#define ALTERA_JUART_H
> > +
> > +#include "hw/sysbus.h"
> > +#include "sysemu/char.h"
> > +
> > +/*
> > + * The read and write FIFO depths can be set from 8 to 32,768 bytes.
> > + * Only powers of two are allowed. A depth of 64 is generally optimal
> for
> > + * performance, and larger values are rarely necessary.
> > + */
> > +
> > +#define FIFO_LENGTH 64
> 
> Should probably be a QOM property, no ?

Did not want to mess with dynamic FIFO buffer allocation.


> 
> > +typedef struct AlteraJUARTState {
> > +    SysBusDevice busdev;
> > +    MemoryRegion mmio;
> > +    CharBackend chr;
> > +    qemu_irq irq;
> > +
> > +    unsigned int rx_fifo_pos;
> > +    unsigned int rx_fifo_len;
> > +    uint32_t jdata;
> > +    uint32_t jcontrol;
> > +    uint8_t rx_fifo[FIFO_LENGTH];
> > +} AlteraJUARTState;
> > +
> > +void altera_juart_create(int channel, const hwaddr addr, qemu_irq irq);
> > +
> > +#endif /* ALTERA_JUART_H */
> >
> btw for trivial patches like this, cover letter is not necessary .

Thanks, good to know, makes life easier
Juro