>>>>> "Peter" == Peter Maydell <peter.mayd...@linaro.org> writes:
Peter> On 30 November 2011 03:36, Peter Chubb Peter> <peter.ch...@nicta.com.au> 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
