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

[Bystricky, Juro]

> [...]
> 
> >>> +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.
> 
> This doesn't answer my question at all. How does real hardware behave if
> you read from unaligned address in the register space , ie. offset 0x3 ?
> 

Not sure I understand the question here. Which "real hardware" are we talking 
about?
If "real hardware" contains MMU or MPU then an exception is generated on 
misalign access.

> [...]
> 
> >>> +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.
> 
> Maybe you can check the core code or someone else can jump in an answer
> this.

OK, will look deeper

[...] 

> >>> +#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.
> 
> You probably should, since this is configurable at the FPGA level (in
> QSys), right ?
> 

OK, will implement fifo size as a property


Thanks
Juro