Re: [Qemu-devel] eepro100c additions/changes required for VxWorks 5.4.2 to work

["Reimar Döffinger" <Reimar.Doeffinger@gmx.de>]

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).