qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Marek Vasut
Subject: Re: [Qemu-devel] [PATCH v4] nios2: Add Altera JTAG UART emulation
Date: Thu, 9 Feb 2017 22:04:59 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0

On 02/09/2017 09:53 PM, Bystricky, Juro wrote:
> 
> 
>> On 02/09/2017 07:52 PM, Juro Bystricky wrote:
>>> JTAG UART core eliminates the need for a separate RS-232 serial
>>> connection to a host PC for character I/O.
>>
>> And how does this describe the content of this patch ? This patch adds a
>> model of JTAG UART , it doesn't eliminate anything ...
>>
> 
> The commit message explicitly says:
> "Add Altera JTAG UART emulation."
> 
> But I will amend the message with more details.

The commit message is totally misleading though.

>>> Hardware emulation based on:
>>> https://www.altera.com/en_US/pdfs/literature/ug/ug_embedded_ip.pdf
>>> (Please see "Register Map" on page 65)
>>>
>>> Signed-off-by: Juro Bystricky <address@hidden>
>>> ---
>>
>> Changelog is missing ...
> 
> I modelled this patch based on other nios2 patches, none of them contained 
> Changelog. So please elaborate.

It's common courtesy to avoid wasting reviewers time by adding a
changelog between patch versions.

>>> +    /*
>>> +     * FIFO size can be set from 8 to 32,768 bytes.
>>> +     * Only powers of two are allowed.
>>> +     */
>>> +    fifo_size_ok = false;
>>> +    for (size = 8; size <= 32768; size *= 2) {
>>> +        if (size == fifo_size) {
>>> +            fifo_size_ok = true;
>>> +            break;
>>> +        }
>>> +    }
>>
>> Isn't that like
>>
>> if (size < 8 || size > 32768 || (size & ~(1 << ffs(size))))
>>   error()
>>
> 
> possibly, however ffs is rejected by checkpatch as non-portable
> libc call. 

Doesn't it suggest to use ctz32() ?
https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03661.html

>>> +#define DEFAULT_FIFO_SIZE 64
>>
>> This is still not QOM property , why ?
>>
> 
> Not sure what you mean, the code has:
> 
> DEFINE_PROP_UINT32("fifo-size", AlteraJUARTState, rx_fifo_size, 
> DEFAULT_FIFO_SIZE),

Ah ok, I missed that (and that should've been part of the changelog for
example ... )

-- 
Best regards,
Marek Vasut



reply via email to

[Prev in Thread] Current Thread [Next in Thread]