From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mewtn-0005qS-Ei for qemu-devel@nongnu.org; Sat, 22 Aug 2009 16:09:15 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mewtj-0005qG-R0 for qemu-devel@nongnu.org; Sat, 22 Aug 2009 16:09:15 -0400 Received: from [199.232.76.173] (port=53523 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mewtj-0005qD-No for qemu-devel@nongnu.org; Sat, 22 Aug 2009 16:09:11 -0400 Received: from mail.gmx.net ([213.165.64.20]:38748) by monty-python.gnu.org with smtp (Exim 4.60) (envelope-from ) id 1Mewti-0002Ad-J0 for qemu-devel@nongnu.org; Sat, 22 Aug 2009 16:09:10 -0400 Date: Sat, 22 Aug 2009 22:09:06 +0200 From: Reimar =?iso-8859-1?Q?D=F6ffinger?= Subject: Re: [Qemu-devel] eepro100c additions/changes required for VxWorks 5.4.2 to work Message-ID: <20090822200906.GA3456@1und1.de> References: <0A54BACCF270364FBD51EAC504A0F0670F8880EA@FRMRSSXCHMBSC2.dc-m.alcatel-lucent.com> <20090822155552.GA9800@1und1.de> <0A54BACCF270364FBD51EAC504A0F067109713D8@FRMRSSXCHMBSC2.dc-m.alcatel-lucent.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <0A54BACCF270364FBD51EAC504A0F067109713D8@FRMRSSXCHMBSC2.dc-m.alcatel-lucent.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org I am only looking at this because I hacked this code, not because I have anything to say on whether it gets in... On Sat, Aug 22, 2009 at 06:42:55PM +0200, ZAPPACOSTA, Rolando (Rolando) wrote: > yes, you are correct, some of them could be considered cosmetic (I added them initially for a better tracking of the emulation each time I launched it). > 1) > @@ -257,6 +266,8 @@ > /* Temporary data. */ > eepro100_tx_t tx; > uint32_t cb_address; > + /* used to store the partial values of the pointer before calling eepro100_write_port */ > + uint16_t port_lo_word; > > /* Statistical counters. */ > eepro100_stats_t statistics; > ===========> REASON: because VxWorks access it as two dwords and the emulation fails if this is not implemented. I don't really understand that, the values always already gets stored to and restored from ->mem... > @@ -782,27 +793,26 @@ > TRACE(RXTX, logout > ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n", > tbd_array, tcb_bytes, s->tx.tbd_count)); > - assert(!(s->tx.command & COMMAND_NC)); > - assert(tcb_bytes <= sizeof(buf)); > + if (s->tx.command & COMMAND_NC) { > + logout("support for NC=1 is not implemented\n"); > + assert (0); > + } > + if (tcb_bytes > sizeof(buf)) { > + logout("illegal value of the TCB byte count! (cannot be greater than 0x%04x)\n", sizeof(buf)); > + assert (0); > + } That part I meant with cosmetics, I think it would simplify things to do it at best in a different patch. Btw. there is a huge amount of trailing whitespace after that } > if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) { > logout > ("illegal values of TBD array address and TCB byte count!\n"); > } > - for (size = 0; size < tcb_bytes; ) { > - uint32_t tx_buffer_address = ldl_le_phys(tbd_address); > - uint16_t tx_buffer_size = lduw_le_phys(tbd_address + 4); > - //~ uint16_t tx_buffer_el = lduw_le_phys(tbd_address + 6); > - tbd_address += 8; > - TRACE(RXTX, logout > - ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n", > - tx_buffer_address, tx_buffer_size)); > - assert(size + tx_buffer_size <= sizeof(buf)); > - cpu_physical_memory_read(tx_buffer_address, &buf[size], > - tx_buffer_size); > - size += tx_buffer_size; > + if (tcb_bytes > 0) { > + TRACE(RXTX, logout("TCB byte count>0, adding the data after the TCB to the buffer\n")); > + cpu_physical_memory_read(s->cb_address + 0x10, &buf[0], tcb_bytes); Lots of trailing whitespace here too. And s->cb_address + 0x10 was already calulated before and stored in tbd_address. Also if extended TCB is enabled I think it must be + 0x20. I admit that the Intel specification says differently but that does not make any sense (I suspect they just did not consider that case)... Apart from that I think this is correct, reading the specification. > @@ -918,6 +931,9 @@ > /* Starting with offset 8, the command contains > * 64 dwords microcode which we just ignore here. */ > break; > + case CmdDiagnose: > + TRACE(OTHER, logout(" Rolando: diagnose\n")); > + break; > default: > missing("undefined command"); > } > ===========> REASON: even though diagnose is called by VxWorks, it should work without this (I realized that later). No, it will crash with an assert due to the missing(...), as said I already sent a patch myself (that sets status to 0). > 4) > @@ -1079,9 +1095,9 @@ > return val; > } > > -static void eepro100_write_eeprom(eeprom_t * eeprom, uint8_t val) > +static void eepro100_write_eeprom(eeprom_t * eeprom, uint32_t val) > { > - TRACE(OTHER, logout("val=0x%02x\n", val)); > + TRACE(OTHER, logout("val=0x%08x\n", val)); > > /* mask unwriteable bits */ > //~ val = SET_MASKED(val, 0x31, eeprom->value); > ===========> REASON: I don't remember exactly what part was requiring this but my emulation failed without this change. If interested, I could change this back and tell you what fails. Except for a compiler bug I can't see how it could make a difference... > 6) > @@ -1484,6 +1504,23 @@ > case SCBeeprom: > eepro100_write_eeprom(s->eeprom, val); > break; > + case SCBPointer: > + s->pointer = (s->pointer & 0xffff0000) + val; > + logout(" Rolando: wrote the General Pointer, it's now 0x%08x\n", s->pointer); > + break; > + case SCBPointer+2: > + s->pointer = (s->pointer & 0xffff) + ( val * 0x10000 ); > + logout(" Rolando: wrote the General Pointer, it's now 0x%08x\n", s->pointer); > + break; I guess reasonable in principle (though it might be simpler to get rid of the pointer variable and just read it whenever necessary form s->mem), except that you need to use le16_to_cpu on val I think. (val << 16) seems better to me than (val * 0x10000).