Re: [Qemu-devel] [PATCH V3 1/4] Implement the FreeScale i.MX UART. This uart is used in a variety of SoCs, including some by Motorola, as well as in the FreeScale i.MX series.

[Peter Chubb]

>>>>> "Peter" == Peter Maydell <address@hidden> writes:

Peter> On 30 November 2011 03:36, Peter Chubb
Peter> <address@hidden> wrote: Commit messages should be
Peter> formatted with a short summary line, then a blank line, then a
Peter> more detailed description. You've put everything into one
Peter> extremely long summary line here, which looks odd in git
Peter> log. Try:

Thanks I've fixed that now.

>> top-level directory.

Peter> Still GPL v2 only.

Fixed.


>> +    uint32_t ubrm;

Peter> The MCIMX31RM calls this UBMR, not UBRM...

Fixed.


Peter> My copy of the MCIMX31RM says we also set RXDS on reset.

Fixed.

>> +    s->usr2 = USR2_TXFE | USR2_TXDC;

Peter> Presumably we don't set DCDIN here because we haven't
Peter> implemented the modem control signals yet?

Exactly.  But there's no harm in setting it, so I've added it in.

>> +    s->uts1 = UTS1_RXEMPTY | UTS1_TXEMPTY; +    s->ubrm = 0; +  
>>  s->ubrc = 0;

Peter> RM says the reset value for UBRC is 0x4.

OK.

Peter> You also need to reset s->ucr1. (If you want to retain that
Peter> hack in the init function then you still need to reset the
Peter> other bits even if you don't allow reset to clear UARTEN.)

OK.  Fixed.

>> +    s->readbuff = 0; 
>> +    imx_update(s);

Peter> This will call qemu_set_irq() which is a bad idea in a reset
Peter> function.  Don't call imx_update() here, instead call it after
Peter> calling imx_serial_reset() from your register write function.

Does the infrastructure guarantee that any interrupt will be cleared
when the reset function is called?

>> +   case  0x21: /* UCR2 */ 
>> +        return 1; /* reset complete */

Peter> UCR2_SRST.

Fixed.


>>    case 0x20: /* UCR1 */ 
>> +        s->ucr1 = value;

Peter> RM says the top 16 bits of UCR1 are read-only.

Fixed.

Peter> This doesn't implement writing to any of the other bits of
Peter> UCR2.

No, apart from the TXEN and RXEN bits they're all to do with 

>> +    case 0x26: /* USR2 */ 
>> +       /* 
>> +        * Writing 1 to some bits clears them; all other 
>> +        * values are ignored 
>> +        */ 
>> +        value &= (1<<15) | (1<<13) | (1<<12) |  (1<<11) | (1<<10)| 
>> +            (1<<8) | (1<<7) | (1<<6) | (1<<4) | (1<<2) | (1<<1);

Peter> define and use USR2_FOO named constants.

Done.


>> UCR3 */ +    case 0x23: /* UCR4 */ +    case 0x24: /* UFCR */ +  
>>  case 0x25: /* USR1 */ +    case 0x2c: /* BIPR1 */ +        /* TODO
>> */

Peter> No IPRINTF() ?

This one gets hit too often.

>> +    .qdev.size  = sizeof (imx_state),

Peter> Unnecessary space (and checkpatch will complain).

I disagree -- there should be a space between the sizeof operator and
the cast that is its operand.  sizeof is not a function; and the
parentheses in this case are part of its operand.

I couldn't find any mention of sizeof in the Coding Style
document. However, I'll go with whatever makes you happier.... (but
note that the current qemu source mixes sizeof (type) and sizeof(type)
all over the place).

Look out for a new patch series real soon now...

Peter C
--
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
http://www.ertos.nicta.com.au           ERTOS within National ICT Australia