On Thursday 20 July 2006 23:57, Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Thu, 20 Jul 2006 13:43:32 +0400
> >
> > This patch finishes the flash support work by adding documentation for
> > all new packets, and by describing the format of XML memory map used in
> > the remote protocol.
>
> Thanks.  My comments are below.
>
> >              @value{GDBN} groups flash memory programming operations
> > +together, and sends a @samp{vFlashDone} request after each group; the
> > +to delay erase operation until the @samp{vFlashDone} packet is received.
>
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Something is wrong here.

Yes, this is the revised wording:

 @value{GDBN} groups flash memory programming operations
together, and sends a @samp{vFlashDone} request after each group; the
stub is allowed to delay erase operation until the @samp{vFlashDone}
packet is received.

> > +Direct the stub to write data to flash address @var{addr}.  The data
> > +is passed binary form using the same encoding as for the @samp{X}
>
>       ^^^^^^^^^^^^^^^^^^
> You meant "passed IN binary form", right?

Right.

>
> >                                  The set of @samp{vFlashWrite} packets
> > +preceding a @samp{vFlashDone} packet must not overlap
>
> It is not clear what does ``must not overlap'' mean here.  Taken
> verbatim, the sentence seems to say that the _packets_ must not
> overlap, but is that really what you want to say?

No, I meant something more to this:

The memory ranges specified by
@samp{vFlashWrite} packets preceding a @samp{vFlashDone} packet must
not overlap, and must appear in order of increasing addresses.

> >                                  Writes may fall outside the regions
> > +given by the previously transmitted @samp{vFlashErase} packets, but
> > +the results are unpredictable if a given area of flash is rewritten
> > +without being erased.
>
> I'd rephrase this as follows:
>
>   If the series of packets write data outside the region erased by the
>   preceding @samp{vFlashErase} packets, the results are unpredictable.

I think this is slightly different. For example, target might have some extra 
fast method to erase entire flash, that gdb does not know about, but that 
user can invoke in some. Then, it's ok to send vFlashWrite without any 
vFlashErase packets at all. So the constraint is that area of flash is in 
erased state when vFlashWrite arrives, but not that vFlashErase was seen.

> > +@item E.memtype
> > +for vFlashWrite addressing non-flash memory
>
> What is "E.memtype"? is it a literal string?

Yes, it's a literal string, just like "OK" two lines above.

> > +@table @samp
> > +@item qXfer:memory-map:read::@var{offset},@var{length}
> > +@anchor{qXfer memory map read}
> > +Access the target's @dfn{memory-map}.  @xref{Memory map format}. The
>
>                                                                   ^^^
> Two spaces here, please.

Ok.

>
> > +@var{annex} must be empty.
>
> There's no @var{annex} in the @item that gives the packet's syntax.

I must admit that I've copied this sentence from description of 
qXfer:auxv:read. While you're right that @var{annex} is not present in 
description of qXfer:memory-map:read, it's present in description of generic 
syntax of qXfer packet. The point of this sentence is to emphasise that annex
part of generic qXfer packet should be empty for memory map.

>
> > +by suppling an appropriate @samp{qSupported} response
> > (@pxref{qSupported}).
>
>       ^^^^^^^^
> A typo.
>
> > +@node Memory map format
> > +@section Memory map format
> > +@cindex Memory map format
>
> Please start @cindex entries with a lower-case letter.
>
> > +lists memory regions. The top-level structure of the document is shown
> > below:
>
>                        ^^^
> Need two spaces here.
>
> > +@example
>
> Please use @smallexample everywhere.

What is the difference between them? Do you mean thatsmallexample should be 
used everywhere in gdb docs? 

> > +@itemize
> > +
> > +@item A region of RAM starting at @var{addr} and extending for
> > @var{length}
>
> An @item in an @itemize list should be alone on its line; the text
> should start on the following line, like this:
>
>   @item
>   A region of RAM starting at @var{addr} and extending ...
>
> > +<memory type="ram" start="addr" length="length"/>
>
> Shouldn't this end with "/memory>"?

No, there's "/>" at the end of the element. That's abbreviated XML syntax for 
element without children.

>
> > +@example
> > +<memory type="rom" start="addr" length="length"/>
> > +@end example
> > +
> > +
> > +@item A region of flash memory, with erasure blocks @var{blocksize}
> > +bytes in length:
> > +
> > +@example
> > +<memory type="flash" start="addr" length="length">
> > +  <property name="blocksize">blocksize</property>
> > +</memory>
> > +@end example
>
> The "addr", "blocksize" and "length" here refer to the values of
> @var{addr}, @var{blocksize}, etc., right?  If so, you should use @var
> in them.

Ok.

>
> > +Regions must not overlap. @value{GDBN} assumes that areas of memory not
> > covered
>
>                           ^^^^
> Two spaces.
>
> > +@example
> > +<!-- ............................................................... -->
> > +<!-- Memory Map XML DTD ............................................ -->
> > +<!-- File: memory-map.dtd .......................................... -->
> > +<!-- ............................................................... -->
>
> Please make these lines shorter: they will overflow the page margins,
> even with @smallexample, since TeX doesn't hyphenate inside @example.
> In general, anything longer than 64 characters in a @smallexample is
> bad.

I've trimmed this to 60 characters.

I attach the revised patch, is it better now?

Thanks,
Volodya