This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] PowerPC - Add a faster way to read the Time Base register


Some simple nits below.  Fix those up and then it's OK to commit.
The manual bits are still imperfect, but we can improve them later.

> +@node Adding Platform-specific
> +@appendixsubsec Platform-specific types, macros and functions

Throughout this section, say "header file", not "header".

> +@item
> +Each included symbol must have the prefix @code{__@var{arch}_}, such as
> +@code{__ppc_get_timebase}.
> +
> +@end itemize

Drop that blank line since there isn't one after @itemize.

> --- /dev/null
> +++ b/manual/platform.texi
> @@ -0,0 +1,32 @@
> +@node Platform, Contributors, Maintenance, Top
> +@c %MENU% Describe all platform-specific facilities provided
> +@appendix Summary of platform-specific facilities

It's not a summary, it's the only documentation there is.

> +@Theglibc{} can provide machine-specific functionality.
> +@ref{Adding Platform-specific} describes the process to include
> +these facilities in @theglibc{} and whether they should be included in
> +GCC as well.

This is a part of the manual for users, who may have no interest in ever
working on libc.  I don't think it should have an xref to something maint.texi.

> +@menu
> +* PowerPC::           Summary of Facilities Specific
> +                                   to the PowerPC Architecture
> +@end menu

Not a summary.

> +The @dfn{Time Base Register} is a 64-bit register that stores a monotonically
> +incremented value updated at a system-dependent frequency that may be
> +different from the processor frequency. More information in Power
> + ISA 2.06b - Book II - Section 5.2.

Two spaces between sentences.  Use proper grammar (that sentence no verb).
Use @cite.

> +/* Read the Time Base Register   */
> +static inline uint64_t
> +__ppc_get_timebase (void)

Period at the end of that sentence.

> +{
> +#ifdef __powerpc64__
> +  uint64_t __tb;
> +  /* "volatile" is necessary here, because the user expects this assembly
> +     isn't moved after an optimization.  */
> +  __asm__ volatile (
> +		    "mfspr %0, 268"
> +		    : "=r" (__tb));

We don't usually break the line after the ( like that.
This whole thing could be on line, so just do that.

> +  uint32_t __tbu, __tbl, __tmp; \
> +  __asm__ volatile (
> +		    "0:\n\t"

Likewise, here put "0..." right after the (.


Thanks,
Roland


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]