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

[Marek Vasut]

On 02/09/2017 04:22 PM, Bystricky, Juro wrote:
> 
> 
>> [...]
>>
>>>>> +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?

By real hardware I mean real Nios2 system ...

> If "real hardware" contains MMU or MPU then an exception is generated on 
> misalign access.

Is this handled here or not ?

>> [...]
>>
>>>>> +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
> 


-- 
Best regards,
Marek Vasut