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

[馬克泡]

Dear Wolfgang,

2011/10/7 Wolfgang Denk <wd at denx.de>:
> Dear =?UTF-8?B?6aas5YWL5rOh?=,
>
>> However, we've discussed before, to support a new architecture, there
>> are some definitions
>> and codes which the checkpatch rules cannot be adapted.
>
> First, we can adapt chackpatch rules.  It just needs to be done.
>
>> We've also discussed that Linux documents said checkpatch is just a
>> reference rule,
>
> I'dlike to see checkpatch customized for our needs.  The tools to do
> that are in place now.
>

Agreed.

>> you still need to have human examination.
>
> Indeed.
>
>> I guess you just forget that because the patch "works" is too heavy
>> and patch check
>> automation didn't help on special case much.
>
> I manually inspected all output before actually sending any messages
> out.
>
>> When we discuss about those was about NDS32 patch v7, and now is patch v15.
>> We've to a lot of fix, includes relocation.
>>
>> So, could you please tell me now, should I resend the patch v16 or you
>> still have my v15
>> patches and kindly tell me which part need to be fixed? Thanks!
>
> There is a large number of WARNING: Use of volatile is usually wrong;
> I think you bit macros and I/O accessors should be cleaned up. I don;t
> want to see volatile there.

About the volatiles and IO accessors, as said and we've discussed
before according to
"Documentation/volatile-considered-harmful.txt". There are still four
kinds of volatiles are still
necessary. Let me copy the exceptions from
"Documentation/volatile-considered-harmful.txt" as follows.

Please give comment on volatiles which are need and should not be get
rid of these. Thanks!

"There are still a few rare situations where volatile makes sense in the
kernel:
- The above-mentioned accessor functions might use volatile on
  architectures where direct I/O memory access does work.  Essentially,
  each accessor call becomes a little critical section on its own and
  ensures that the access happens as expected by the programmer.

- Inline assembly code which changes memory, but which has no other
  visible side effects, risks being deleted by GCC.  Adding the volatile
  keyword to asm statements will prevent this removal.

- The jiffies variable is special in that it can have a different value
  every time it is referenced, but it can be read without any special
  locking.  So jiffies can be volatile, but the addition of other
  variables of this type is strongly frowned upon.  Jiffies is considered
  to be a "stupid legacy" issue (Linus's words) in this regard; fixing it
  would be more trouble than it is worth.

- Pointers to data structures in coherent memory which might be modified
  by I/O devices can, sometimes, legitimately be volatile.  A ring buffer
  used by a network adapter, where that adapter changes pointers to
  indicate which descriptors have been processed, is an example of this
  type of situation."

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)

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))

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");

> 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.
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; })

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) */

WARNING: line over 80 characters (Comment and offset for a SoC).
#933: FILE: arch/nds32/include/asm/arch-ag101/ag101.h:27:
+#define CONFIG_FTSDMC021_BASE          0x90300000      /*
FTSDMC020/021 SDRAM Controller */

WARNING: line over 80 characters (to made this align with other architectures).
#132: FILE: examples/standalone/x86-testapp.c:63:
+       : : "i"(offsetof(xxx_t, pfunc)), "i"(XF_ ## x * sizeof(void
*)) : "$r16");

I can do fix for these lines over 80 characters but I think the add-on
codes might be less readable.

>> The 8 errors is due to the new assembly march.h added.
>> Only 2 new cases introduced.
>>
>> ERROR: space prohibited before open square bracket '['
>> #1032: FILE: arch/nds32/include/asm/macro.h:66:
>> +       lwi     $r5, [$r4]
>> This is to load a 32bit from the address x, while the value of
>> address x is stored in $r4, and load it into $r5
>>
>> ERROR: spaces required around that ':' (ctx:VxE)
>> #1055: FILE: arch/nds32/include/asm/macro.h:89:
>> +1:
>>   ^
>
> I agree that we can ignore these (or rather try and add exception
> rules).
>
>
> Best regards,
>
> Wolfgang Denk

-- 
Best regards,
Macpaul Lin