[U-Boot] [PATCH v15 1/9] nds32: add header files support for nds32

[Wolfgang Denk <wd@denx.de>]

Dear =?UTF-8?B?6aas5YWL5rOh?=,

In message <CACCg+XMDNyxa5-rAdvj=oG8yhmFAkiD8OZzhZE+-ThFXcT6sfQ@mail.gmail.com> you wrote:
>
> I think volatiles which like the following should be necessary
> according to the 4 exceptions listed in the document
> "Documentation/volatile-considered-harmful.txt".
> 
> WARNING: Use of volatile is usually wrong: see
> Documentation/volatile-considered-harmful.txt
> #124: FILE: arch/nds32/include/asm/bitops.h:31:
> +extern void set_bit(int nr, volatile void *addr);
> 
> WARNING: Use of volatile is usually wrong: see
> Documentation/volatile-considered-harmful.txt
> #126: FILE: arch/nds32/include/asm/bitops.h:33:
> +static inline void __set_bit(int nr, volatile void *addr)

If you look at the implementations of these functions, they need
explicit casts to actually get rid of the "volatile" attribute.
So why would there be any need to pass it in the first place?

> WARNING: Use of volatile is usually wrong: see
> Documentation/volatile-considered-harmful.txt
> #594: FILE: arch/nds32/include/asm/io.h:78:
> +#define __arch_getw(a)                 (*(volatile unsigned short *)(a))

I disagree with this code for several reasons.

First, it should be turned into a (inline) function, so the compiler
can actually verify the type of the argument and make sure it is
indeed a 16 bit pointer.

Second, please check and re-check if there is really no kind of memry
barrier instruction needed here.  And even if there is no such need, I
tend belive that adding some __iormb(); similar to whay is done for
example in "arch/arm/include/asm/io.h" would still be a good idea.

If the actual access goes through dereferencing a volatile pointer,
this should be hidden at the lowest level of the implementation.

> WARNING: Use of volatile is usually wrong: see
> Documentation/volatile-considered-harmful.txt
> #142: FILE: examples/standalone/x86-testapp.c:86:
> +       register volatile xxx_t *pq asm("$r16");

I don;t know this code, so I cannot comment onthis.

> > WARNING: line over 80 characters needs to be fixed, obviously.
> 
> Sorry about this, but the 80 characters formatting is for human
> reading. To fix some of the lines exceeds
> 80 characters made me feel the code has become less human readable.

If you don't fix it, then the text will wrap around in a completely
uncontrolled way depending on the window width.  This is even worse.

> For example:
> WARNING: line over 80 characters
> #824: FILE: arch/nds32/include/asm/io.h:308:
> +#define readw(c) ({ unsigned int __v =
> le16_to_cpu(__raw_readw(__mem_pci(c))); __v; })

#define readw(c) ({					\
	unsigned int __v				\
							\
	__v = le16_to_cpu(__raw_readw(__mem_pci(c)))	\
})

> WARNING: line over 80 characters
> #932: FILE: arch/nds32/include/asm/arch-ag101/ag101.h:26:
> +#define CONFIG_FTSMC020_BASE           0x90200000      /* Static
> Memory Controller (SRAM) */

/* Static Memory Controller (SRAM) */
#define CONFIG_FTSMC020_BASE		0x90200000

etc.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's certainly  convenient  the  way  the  crime  (or  condition)  of
stupidity   carries   with   it  its  own  punishment,  automatically
admisistered without remorse, pity, or prejudice. :-)
         -- Tom Christiansen in <559seq$ag1$1@csnews.cs.colorado.edu>