Re: [U-Boot] [PATCH 01/13] Blackfin: BF60x: new processor header files

2012-09-05 Thread Bob Liu
Hi Wolfgang,

Thank you for your review.

On Sun, Sep 2, 2012 at 10:10 PM, Wolfgang Denk w...@denx.de wrote:
 Dear Bob Liu,

 In message 1345526833-10804-1-git-send-email-lliu...@gmail.com you wrote:
 Add header files for blackfin new processor bf60x.

 Signed-off-by: Bob Liu lliu...@gmail.com
 ---
  arch/blackfin/include/asm/blackfin_cdef.h |3 +
  arch/blackfin/include/asm/blackfin_def.h  |5 +
  arch/blackfin/include/asm/blackfin_local.h|3 +
  arch/blackfin/include/asm/mach-bf609/BF609_cdef.h |  543 +++
  arch/blackfin/include/asm/mach-bf609/BF609_def.h  | 3758 
 +
  arch/blackfin/include/asm/mach-bf609/anomaly.h|  128 +
  arch/blackfin/include/asm/mach-bf609/def_local.h  |5 +
  arch/blackfin/include/asm/mach-bf609/portmux.h|  257 ++
  arch/blackfin/include/asm/mach-bf609/ports.h  |  103 +
  arch/blackfin/include/asm/mach-common/bits/cgu.h  |   80 +
  arch/blackfin/include/asm/mach-common/bits/dde.h  |   88 +
  arch/blackfin/include/asm/mach-common/bits/mpu.h  |6 +-
  arch/blackfin/include/asm/mach-common/bits/pll.h  |5 +
  13 files changed, 4983 insertions(+), 1 deletion(-)
  create mode 100644 arch/blackfin/include/asm/mach-bf609/BF609_cdef.h
  create mode 100644 arch/blackfin/include/asm/mach-bf609/BF609_def.h
  create mode 100644 arch/blackfin/include/asm/mach-bf609/anomaly.h
  create mode 100644 arch/blackfin/include/asm/mach-bf609/def_local.h
  create mode 100644 arch/blackfin/include/asm/mach-bf609/portmux.h
  create mode 100644 arch/blackfin/include/asm/mach-bf609/ports.h
  create mode 100644 arch/blackfin/include/asm/mach-common/bits/cgu.h
  create mode 100644 arch/blackfin/include/asm/mach-common/bits/dde.h

 Please make sure to have the string PATCH included with all patch
 submissions, otherwise your patches are lost to patchwork, and most
 likely to the respective custodian as well.

 --- /dev/null
 +++ b/arch/blackfin/include/asm/mach-bf609/BF609_cdef.h
 ...
 +#define bfin_read_CGU_STAT() bfin_read32(CGU_STAT)
 +#define bfin_read_CGU_CLKOUTSEL() bfin_read32(CGU_CLKOUTSEL)
 +#define bfin_read_CGU_CTL() bfin_read32(CGU_CTL)
 +#define bfin_write_CGU_CTL(val) bfin_write32(CGU_CTL, val)
 +#define bfin_read_CGU_DIV() bfin_read32(CGU_DIV)
 +#define bfin_write_CGU_DIV(val) bfin_write32(CGU_DIV, val)

 We don't allow CamelCaps identifiers.  Please fix globally.


Sorry, i didn't get your idea here.


 +#define CNT_CFG 0xFFC00400 /* CNT0 
 Configuration Register */
 +#define CNT_IMSK0xFFC00404 /* CNT0 Interrupt 
 Mask Register */
 +#define CNT_STAT0xFFC00408 /* CNT0 Status 
 Register */
 +#define CNT_CMD 0xFFC0040C /* CNT0 Command 
 Register */
 +#define CNT_DEBNCE  0xFFC00410 /* CNT0 Debounce 
 Register */
 +#define CNT_CNTR0xFFC00414 /* CNT0 Counter 
 Register */
 +#define CNT_MAX 0xFFC00418 /* CNT0 Maximum 
 Count Register */
 +#define CNT_MIN 0xFFC0041C /* CNT0 Minimum 
 Count Register */

 We don't allow register access based on raw addresses or base address
 plus offset.  Please define proper C structs to describe your
 hardware.   Please fix globally.


C structs can be defined to access these registers in other place.
But this file is come from our hardware team i can't change it.


 --- /dev/null
 +++ b/arch/blackfin/include/asm/mach-bf609/anomaly.h
 @@ -0,0 +1,128 @@
 +/*
 + * DO NOT EDIT THIS FILE
 + * This file is under version control at
 + *   
 svn://sources.blackfin.uclinux.org/toolchain/trunk/proc-defs/header-frags/
 + * and can be replaced with that version at any time
 + * DO NOT EDIT THIS FILE

 This is bullshit.  If you submit code to U-Boot, it gets maintained in
 the U-Boot git repository.  Dump this.

 + * Copyright 2004-2010 Analog Devices Inc.
 + * Licensed under the ADI BSD license.
 + *   https://docs.blackfin.uclinux.org/doku.php?id=adi_bsd

 Is this GPL compatible??

 The link is dead and returns only

 This topic does not exist yet

 +#define ANOMALY_0574 (1)

 Please do not put parens around simple defines.  Please fix globally.


Will be fixed.

 Checkpatch throws a ton of errors that all need fixing.


Which Checkpatch script are you using?
In my test there are only WARNING: line over 80 characters.

total: 0 errors, 3546 warnings, 5018 lines checked

NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
MULTISTATEMENT_MACRO_USE_DO_WHILE

/home/bob/u-boot2/0001-Blackfin-BF60x-new-processor-header-files.patch
has style problems, please review.

 Also note that we don't allow fixed network parameters (like
 CONFIG_ETHADDR) in board config files.


Will be fixed.

 Review stops here (except for the last part with the licensing stuff).


Thank you !

-- 
Regards,
--Bob
___
U-Boot mailing list

Re: [U-Boot] [PATCH 01/13] Blackfin: BF60x: new processor header files

2012-09-02 Thread Wolfgang Denk
Dear Bob Liu,

In message 1345526833-10804-1-git-send-email-lliu...@gmail.com you wrote:
 Add header files for blackfin new processor bf60x.
 
 Signed-off-by: Bob Liu lliu...@gmail.com
 ---
  arch/blackfin/include/asm/blackfin_cdef.h |3 +
  arch/blackfin/include/asm/blackfin_def.h  |5 +
  arch/blackfin/include/asm/blackfin_local.h|3 +
  arch/blackfin/include/asm/mach-bf609/BF609_cdef.h |  543 +++
  arch/blackfin/include/asm/mach-bf609/BF609_def.h  | 3758 
 +
  arch/blackfin/include/asm/mach-bf609/anomaly.h|  128 +
  arch/blackfin/include/asm/mach-bf609/def_local.h  |5 +
  arch/blackfin/include/asm/mach-bf609/portmux.h|  257 ++
  arch/blackfin/include/asm/mach-bf609/ports.h  |  103 +
  arch/blackfin/include/asm/mach-common/bits/cgu.h  |   80 +
  arch/blackfin/include/asm/mach-common/bits/dde.h  |   88 +
  arch/blackfin/include/asm/mach-common/bits/mpu.h  |6 +-
  arch/blackfin/include/asm/mach-common/bits/pll.h  |5 +
  13 files changed, 4983 insertions(+), 1 deletion(-)
  create mode 100644 arch/blackfin/include/asm/mach-bf609/BF609_cdef.h
  create mode 100644 arch/blackfin/include/asm/mach-bf609/BF609_def.h
  create mode 100644 arch/blackfin/include/asm/mach-bf609/anomaly.h
  create mode 100644 arch/blackfin/include/asm/mach-bf609/def_local.h
  create mode 100644 arch/blackfin/include/asm/mach-bf609/portmux.h
  create mode 100644 arch/blackfin/include/asm/mach-bf609/ports.h
  create mode 100644 arch/blackfin/include/asm/mach-common/bits/cgu.h
  create mode 100644 arch/blackfin/include/asm/mach-common/bits/dde.h

Please make sure to have the string PATCH included with all patch
submissions, otherwise your patches are lost to patchwork, and most
likely to the respective custodian as well.

 --- /dev/null
 +++ b/arch/blackfin/include/asm/mach-bf609/BF609_cdef.h
...
 +#define bfin_read_CGU_STAT() bfin_read32(CGU_STAT)
 +#define bfin_read_CGU_CLKOUTSEL() bfin_read32(CGU_CLKOUTSEL)
 +#define bfin_read_CGU_CTL() bfin_read32(CGU_CTL)
 +#define bfin_write_CGU_CTL(val) bfin_write32(CGU_CTL, val)
 +#define bfin_read_CGU_DIV() bfin_read32(CGU_DIV)
 +#define bfin_write_CGU_DIV(val) bfin_write32(CGU_DIV, val)

We don't allow CamelCaps identifiers.  Please fix globally.


 +#define CNT_CFG 0xFFC00400 /* CNT0 Configuration 
 Register */
 +#define CNT_IMSK0xFFC00404 /* CNT0 Interrupt 
 Mask Register */
 +#define CNT_STAT0xFFC00408 /* CNT0 Status 
 Register */
 +#define CNT_CMD 0xFFC0040C /* CNT0 Command 
 Register */
 +#define CNT_DEBNCE  0xFFC00410 /* CNT0 Debounce 
 Register */
 +#define CNT_CNTR0xFFC00414 /* CNT0 Counter 
 Register */
 +#define CNT_MAX 0xFFC00418 /* CNT0 Maximum Count 
 Register */
 +#define CNT_MIN 0xFFC0041C /* CNT0 Minimum Count 
 Register */

We don't allow register access based on raw addresses or base address
plus offset.  Please define proper C structs to describe your
hardware.   Please fix globally.


 --- /dev/null
 +++ b/arch/blackfin/include/asm/mach-bf609/anomaly.h
 @@ -0,0 +1,128 @@
 +/*
 + * DO NOT EDIT THIS FILE
 + * This file is under version control at
 + *   
 svn://sources.blackfin.uclinux.org/toolchain/trunk/proc-defs/header-frags/
 + * and can be replaced with that version at any time
 + * DO NOT EDIT THIS FILE

This is bullshit.  If you submit code to U-Boot, it gets maintained in
the U-Boot git repository.  Dump this.

 + * Copyright 2004-2010 Analog Devices Inc.
 + * Licensed under the ADI BSD license.
 + *   https://docs.blackfin.uclinux.org/doku.php?id=adi_bsd

Is this GPL compatible??

The link is dead and returns only

This topic does not exist yet

 +#define ANOMALY_0574 (1)

Please do not put parens around simple defines.  Please fix globally.

Checkpatch throws a ton of errors that all need fixing.

Also note that we don't allow fixed network parameters (like
CONFIG_ETHADDR) in board config files.

Review stops here (except for the last part with the licensing stuff).

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: w...@denx.de
All these theories, diverse as they are, have two things  in  common:
they explain the observed facts, and they are completeley and utterly
wrong.   - Terry Pratchett, _The Light Fantastic_
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot