RE: [oc] WISHBONE DMA/Bridge

[John Dalton <john.dalton@bigfoot.com>]

Hi Rudi,

I have just finished reading the WISHBONE DMA/Bridge IP Core 
documentation.

As I read the document, I put together a list of errata, and 
suggested changes.  I hope it is of use.  Please take it
as constructive criticism.  It's fairly long, but I hope I don't
pontificate too much.

Thanks for putting the effort into writing decent documentation.
I really do think decent documentation is as important as HDL.

Best wishes
John

--------------------------


An explanation of the terminology I use to describe locations in 
the document:
X.X pN line M  refers to line M of paragraph N within section X.X

I have prioritised each point according to
Error:       A thing that should be fixed, like a spelling mistake
Suggestion:  Something that is correct, but I feel can be improved
Nit Picking: Trivial stuff



1) 2.2: p1 line1: Error

"engine that support transfers"

should be

"engine that supports transfers"


2) 2.2: p1 line2: Suggestion

replace "and" with "as well as"


3) 2.3: Suggestion

"This block performs the pass through operation..."

does not convey any additional information, given that
the block is named "pass through".  How about (borrowing from 
section 3.4)

"This block performs a bridging operation..."


4) Chapter 3:  Suggestion

Figure 2 is not referred to in the text.  Should it be, to remove 
all possible ambiguity as to where it sits in the document?  
Perhaps append ", as in Figure 2" to the end of the sentence above 
the figure?


5) Chapter 3:  Suggestion

"DMA Engine" is used in two contexts.  In one context (first 
sentence of chapter 3, and caption of figure 2), it refers to the 
"WISHBONE DMA/Bridge" as a whole.  In another context (second half 
of first sentence of chapter 3, block within figure 2, and 3.2), 
it refers to a sub system within the "WISHBONE DMA/Bridge".

Using the same term in different contexts has potential to 
confuse, especially if English is not one's first language.


6) 3.1 p2 line 1:  Error

"Below figure"

should be

"The figure below", or better still "Figure 3..."

"Below figure" is not correct English.  Apologies if this is 
correct usage in the US, but I would argue that we should try to 
write for a global audience.

Given that we are at the bottom of a page, it is strictly
incorrect to refer to the figure "below".

Finally, if the figure shifts in a newer version of the document, 
it may no longer be below.  Perhaps it i sbetter to refer to 
figures and table by number? (use references to keep figure 
numbers automatically up to date).


7) 3.2.1 p1: Suggestion

"Below figure"

should be

"Figure 4"

for the same reasons as in 6).


8) 3.2.1 p2 line 2: Error

"on a different interfaces."

should be

"on a different interface."


9) 3.2.1 p2 line 3: Suggestion

A reference "CYC_O" pops out of nowhere.  Should this
sentence be read in conjunction with source code or the wishbone 
standard?
I suggest that
1) The user be explicitly referred to the source code/standard, or
2) Quote any source code to keep the document self contained, or
3) Show this signal on a block diagram


10) 3.2.1: Suggestion

Related to point 9), lots of register names and signals pop up 
unexpectedly:

CSR, auto restart bit, CHK_SZ, TXSZ, USE_ED, EOL, 

The context and function of these signals/registers should be 
defined.


11) 3.2.1 p2 line 7: Error

"it immediately restart the operation."

should be

"it immediately restarts the operation."


12) 3.2.1 p3 sentence 1: Suggestion

"If CHK_SZ is not zero, the channel has to re-arbitrate for the 
interfaces after each CHK_SZ of word have been transferred."

The meaning of this is not clear to me.  I gather it has something 
to do with the time period each channel gets when doing a round 
robin?


13a) Figure 6:  Suggestion

Change the heading for the table, within the figure, to something 
like: "Definition of bits in the DESC_CSR word:"


13b) Figure 6: Add offsets (base, base+1, base+2, base+3) to 
indicate which word is highest in memory.


14) 3.3 p2 line 2: Suggestion


"The chunk size must also be programed in to the desired value in 
the channel TXSZ register."

Does not make complete sense.  Change it to

"The chunk size must also be set to the desired value in the 
channel TXSZ register."


15) Various places

The word "programmed" has been incorrectly spelt as "programed".  
Apologies again if this is a US spelling, but the spell checker on 
my word processor rejects it when set to 'US'.


16) 3.6 p1 line 3: Error

"zero, the TOT_SZ number of word will be transferred."

should be

"zero, the TOT_SZ number of words will be transferred."

or is

"zero, TOT_SZ number of words will be transferred.

clearer?


17) Figure 8: Suggestion

I presume the solid gray bar indicates a missing chunk of time?  
Is this a wide spread way of indicating missing time?

Normally I associate two vertical wiggly lines with missing time.
Granted this is harder to draw.  If a gray bar is your own 
convention, perhaps it should be explained in the text, or a 
footnote?

18)  3.7 p1 sentence 2: Suggestion

The meaning of this sentence is not clear to me


19) 3.7 p4 line 1: Suggestion

Change

"The first way, the NDn_I signal is asserted"

to

"The first way is to assert the NDn_I signal"


20) 3.7 p4 sentence 2: Suggestion

There are too many "nexts" and "currents" here.  I get lost.
Can you break up the sentence, or make it simpler? Try changing:

"In this case the descriptor for the channel will be invalidated 
and marked as "serviced" and when the next REQn_I is asserted the 
next descriptor will be fetched as indicated by the next pointer 
in the current descriptor."

to

"In this case the descriptor for the channel will be invalidated 
and marked as "serviced". When the REQn_I is asserted the next 
descriptor will be fetched from the address pointed to by the 
current descriptor."


21) 3.7 p4 line 5:  Error

Change

"descriptors"

to 

"descriptor's"

(missing apostophe)



22) 3.7 p4 line 6:  Error

Change

"channels"

to 

"channel's"

(missing apostophe)


23) 3.7 p4 last sentence: Suggestion

"Software has to reset the DESCn register and the channel CSR 
register."

This sentence pops out of nowhere.  Perhaps some more background?


24) 3.7 general comments

This section is quite convoluted with lots of signal names and 
acronyms floating around.  It does make sense, but is very 
difficult to follow.  The whole thing could be simplified by 
adding two timing diagrams.  The text could then walk the reader
through the timing diagrams.

Be consistent with your terms. For example in p5 line 4: "If the 
SZ_WB in the channel CSR is set," would be clearer if written "If 
the SZ_WB bit is set in the channel CSR register," Some places you 
refer to the "CSR", while in other places you refer to the "CSR 
register".  I know "CSR register" is a tautology if you expand the 
acronym, but it does improve the readability.


25) 3.7 p5 line 4: Error

"by marking is "serviced"."

should be

"by marking it "serviced"."


26) Table 1: Error?

Are the address fields for DESC0 to DESC3 right?


27) Section 4 p1 line 1: Error

"status register inside"

should be

"status registers inside"


28) Section 4: Suggestion (nit picking)

Can you make it more obvious that you define all registers at the 
start of section 4, then go on to define the bits within each 
register in each sub section?  After reading "This section 
describes all control and status register inside the WISHBONE 
DMA/Bridge core." I was expecting 4.1 and so on to be defining 
more registers. Instead I had to figure out that there was a 
hierarchy here and you were defining the bits within the registers 
just described.  It's a small thing, but it did break my train of 
thought while reading.


29) Suggestion

Specify that, if written to, any reserved bit is to be written 
with some default value (typically zero). This allows future 
hardware extensions to be compatible with existing software.  If 
the hardware defines a new function for a reserved bit, the 
previously defined default value should reproduce the existing 
hardware functionality.

Similarly, specify that all reserved bits are undefined when read, 
to force software to mask them out.


30) 4.1 (and rest of section 4): Nit picking

Change "Reset Value:"

to "Value after reset:", or "Default value:"


31) 4.2 p1 line 1: Suggestion

Change

"functionality of INT_A and INT_B"

to

"functionality of the INT_A and INT_B"


32) 4.2 p1 line 2: Error

Put a space after the full stop.


33) 4.2 p1: Suggestion

Be consistent with use of numbers:  "1" vs. "one", "0" vs. zero.
I think it is strictly correct to spell out such numbers in full.


Extra Comments

The document does not define many of the terms that it uses.
For example, DMA, IP and so on.  Is this document supposed to
stand alone?  If so, should it give a brief background on DMA?

I will now answer my own question and say no it shouldn't.
Nothing seems more silly to me than having to wade through
pages of distracting "standard" information (acronym definitions, 
disclaimers and so on) to get to the core of a document.

Perhaps opencores should have a glossary and set of tutorials
on common topics, as stand alone documents?  Rudi's document could 
then simply have a  reference section at the back, where he points 
the reader towards additional information, which an experienced 
reader would find distracting.


Would it be possible to place the source for the documentation in 
the CVS repository, and release it under the free documentation 
license? (see http://www.gnu.org for details of this license)
That way someone contributing an updated design can also 
contribute updated documentation.