RE: [ethmac] rx_ethmac

["Illan Glasner" <iglasner@zumanetworks.com>]

13-Feb-01

   Mahmud Hi,

	Just few more comment's :

1.
If I may suggest instead of the long long stimulus file that have :

"
...
run 40              
force rx_data   1011  0
run 40              
force rx_data   1111  0
run 40              
force rx_data   0101  0
run 40              
force rx_data   0011  0
...
"

etc it would be better to use the random function and a loop with random
size this will take only few lines as well as the tester will be much
more robust as it test every run a new pattern.

easiyl you can than add change in preamable size wchihc also will be
random and good/bad sfd random and so on so with few random line you get
a much more verification cover.

oh and don't forget of course to change the seed before each run.


2. 
In the crc module I notice you used function and was wonder if you took
them from the http://www.easics.be/webtools/crctool if so it would be
appropriate if you add a comment in your code that it was taken from
there.

They did a great job for me the save lot's of excell spread sheet work
as I used to find the formula the hard way and they should get the
credit.

on the other hand if it is all from your head than forget what I write.

3.
you might want in reset to always reset all the FF even tho it is not a
must in some cases.

for example in aml file :

"always @(posedge clk or negedge reset_n)
  if (!reset_n) 
    state_AML <= AML_IDLE;
  else"

but also in other file like rx_sm.


4. in aml I assume the AML_STOP is to be used when the DA is latched and
you can do the comparison if so wouldn't this be simpler :

always @(posedge clk or negedge reset_n)
begin
if (!reset_n) 
  begin
     address_match = 1'b0;   
     broadcast     = 1'b0;
     multicast     = 1'b0;  
  end
else if(state_AML == AML_STOP)
  begin
     address_match = (DA == eth_address);   
     broadcast     = (DA == `BROADCAST);
     multicast     = (DA[40]);  
  end
 

instead of :

always @(posedge clk or negedge reset_n)
begin
  if (!reset_n) 
  begin
     address_match = 1'b0;   
     broadcast     = 1'b0;
     multicast     = 1'b0;  
  end
  else
  if(state_AML == AML_IDLE)
  begin
     address_match = 1'b0;   
     broadcast     = 1'b0;
     multicast     = 1'b0;  
  end
  else
     case(DA)
       eth_address : 
               address_match = 1'b1;
       (eth_address | `MULTICAST_SIGN) :
               begin
                 multicast = 1'b1;
                 address_match = 1'b1;
               end
       `BROADCAST : 
             begin
               broadcast = 1'b1;
               address_match = 1'b1;
             end
       default : 
             address_match = 1'b0;
     endcase
   end        


I might made a typo or even I might miss understaood you etc but what I
try to show that if you can write it in fewer line it is more easy to
read and to understand by other what you try to do as well as to debug
it if needed.



5.
Why do you have in the sm so many hold and wait states ? 

I belive that from idle to start you go when rxdv=1 than when sdf=1 you
go to start.
and in all states if rxdv=0 you go to end where you either report the
result or error depend on you. and next clock return to idle or maybe
you can combine end and idle depend on your coding.

have a nice day

   Illan




-----Original Message-----
From: Mahmud Galela [mailto:mgalela@vlsi.itb.ac.id]
Sent: Sunday, February 11, 2001 6:53 PM
To: ethmac@opencores.org
Subject: [ethmac] rx_ethmac


hi,

the code from me is available now.
(version 1)

http://ic.ee.itb.ac.id/~mgalela/isi/ta/rx_ethmac.html
(..i put them here untill the cvs is ok..)



Mahmud