Patchwork [U-Boot,v3,01/23] powerpc, mpc83xx: add missing functions to mpc83xx.h

login
register
mail settings
Submitter Heiko Schocher
Date March 21, 2011, 7:01 a.m.
Message ID <1300690939-31511-2-git-send-email-hs@denx.de>
Download mbox | patch
Permalink /patch/87703/
State Superseded
Headers show

Comments

Heiko Schocher - March 21, 2011, 7:01 a.m.
Signed-off-by: Heiko Schocher <hs@denx.de>
cc: Kim Phillips <kim.phillips@freescale.com>
cc: Holger Brunck <holger.brunck@keymile.com>
cc: Wolfgang Denk <wd@denx.de>
cc: Detlev Zundel <dzu@denx.de>
cc: Valentin Longchamp <valentin.longchamp@keymile.com>
---
Changes for v3:
  - new patch in v3, to avoid externs in keymile code

 include/mpc83xx.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Kim Phillips - March 31, 2011, 12:40 a.m.
On Mon, 21 Mar 2011 08:01:57 +0100
Heiko Schocher <hs@denx.de> wrote:

> Signed-off-by: Heiko Schocher <hs@denx.de>
> cc: Kim Phillips <kim.phillips@freescale.com>
> cc: Holger Brunck <holger.brunck@keymile.com>
> cc: Wolfgang Denk <wd@denx.de>
> cc: Detlev Zundel <dzu@denx.de>
> cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> ---

Hi Heiko, sorry for the late review, but I must admit it doesn't help
the reviewer at all when later patches in a patchseries modify things
added by earlier patches in the same patchseries!

>  include/mpc83xx.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/include/mpc83xx.h b/include/mpc83xx.h
> index ea137c7..e1b0929 100644
> --- a/include/mpc83xx.h
> +++ b/include/mpc83xx.h
> @@ -1274,6 +1274,12 @@ struct pci_region;
>  void mpc83xx_pci_init(int num_buses, struct pci_region **reg);
>  void mpc83xx_pcislave_unlock(int bus);
>  void mpc83xx_pcie_init(int num_buses, struct pci_region **reg);
> +
> +void disable_addr_trans(void);
> +void enable_addr_trans(void);
> +#if defined(CONFIG_DDR_ECC) && !defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)
> +void ddr_enable_ecc(unsigned int dram_size);
> +#endif

I don't believe these prototypes belong in mpc83xx.h - they're really
not 83xx-specific - e.g., 74xx and 85xx have identical names for
functions that have the same...function.

Looking around I think the best place for them would be the 'start.S'
section of include/common.h.  Feel free to protect with 83xx ifdefs;
others can add their platforms as necessary.

Thanks,

Kim
Heiko Schocher - March 31, 2011, 5:38 a.m.
Hello Kim,

Kim Phillips wrote:
> On Mon, 21 Mar 2011 08:01:57 +0100
> Heiko Schocher <hs@denx.de> wrote:
> 
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> cc: Kim Phillips <kim.phillips@freescale.com>
>> cc: Holger Brunck <holger.brunck@keymile.com>
>> cc: Wolfgang Denk <wd@denx.de>
>> cc: Detlev Zundel <dzu@denx.de>
>> cc: Valentin Longchamp <valentin.longchamp@keymile.com>
>> ---
> 
> Hi Heiko, sorry for the late review, but I must admit it doesn't help
> the reviewer at all when later patches in a patchseries modify things
> added by earlier patches in the same patchseries!

Sorry for that, I know, it is a big patchset ...

>>  include/mpc83xx.h |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/mpc83xx.h b/include/mpc83xx.h
>> index ea137c7..e1b0929 100644
>> --- a/include/mpc83xx.h
>> +++ b/include/mpc83xx.h
>> @@ -1274,6 +1274,12 @@ struct pci_region;
>>  void mpc83xx_pci_init(int num_buses, struct pci_region **reg);
>>  void mpc83xx_pcislave_unlock(int bus);
>>  void mpc83xx_pcie_init(int num_buses, struct pci_region **reg);
>> +
>> +void disable_addr_trans(void);
>> +void enable_addr_trans(void);
>> +#if defined(CONFIG_DDR_ECC) && !defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)
>> +void ddr_enable_ecc(unsigned int dram_size);
>> +#endif
> 
> I don't believe these prototypes belong in mpc83xx.h - they're really
> not 83xx-specific - e.g., 74xx and 85xx have identical names for
> functions that have the same...function.
> 
> Looking around I think the best place for them would be the 'start.S'
> section of include/common.h.  Feel free to protect with 83xx ifdefs;
> others can add their platforms as necessary.

Hmm.. I thought of this too, but that will result in adding ifdefs.
(and special this file has a lot of ifdefs, so I wanted to prevent
 another ifdef mess...). But I can of course move it to
include/common.h if thats the preferred place ... ?

bye,
Heiko
Kim Phillips - March 31, 2011, 3:54 p.m.
On Thu, 31 Mar 2011 07:38:18 +0200
Heiko Schocher <hs@denx.de> wrote:

> Hello Kim,
> 
> Kim Phillips wrote:
> > On Mon, 21 Mar 2011 08:01:57 +0100
> > Heiko Schocher <hs@denx.de> wrote:
> > 
> >> Signed-off-by: Heiko Schocher <hs@denx.de>
> >> cc: Kim Phillips <kim.phillips@freescale.com>
> >> cc: Holger Brunck <holger.brunck@keymile.com>
> >> cc: Wolfgang Denk <wd@denx.de>
> >> cc: Detlev Zundel <dzu@denx.de>
> >> cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> >> ---
> > 
> > Hi Heiko, sorry for the late review, but I must admit it doesn't help
> > the reviewer at all when later patches in a patchseries modify things
> > added by earlier patches in the same patchseries!
> 
> Sorry for that, I know, it is a big patchset ...

it's just that it could have been shortened to all the refactoring
parts first, then the adding of all the new boards.

> >>  include/mpc83xx.h |    6 ++++++
> >>  1 files changed, 6 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/mpc83xx.h b/include/mpc83xx.h
> >> index ea137c7..e1b0929 100644
> >> --- a/include/mpc83xx.h
> >> +++ b/include/mpc83xx.h
> >> @@ -1274,6 +1274,12 @@ struct pci_region;
> >>  void mpc83xx_pci_init(int num_buses, struct pci_region **reg);
> >>  void mpc83xx_pcislave_unlock(int bus);
> >>  void mpc83xx_pcie_init(int num_buses, struct pci_region **reg);
> >> +
> >> +void disable_addr_trans(void);
> >> +void enable_addr_trans(void);
> >> +#if defined(CONFIG_DDR_ECC) && !defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)
> >> +void ddr_enable_ecc(unsigned int dram_size);
> >> +#endif
> > 
> > I don't believe these prototypes belong in mpc83xx.h - they're really
> > not 83xx-specific - e.g., 74xx and 85xx have identical names for
> > functions that have the same...function.
> > 
> > Looking around I think the best place for them would be the 'start.S'
> > section of include/common.h.  Feel free to protect with 83xx ifdefs;
> > others can add their platforms as necessary.
> 
> Hmm.. I thought of this too, but that will result in adding ifdefs.

in common.h?  there already is a ifdef 83xx in the start.S section of
that file - at line 447

wrt ddr_enable_ecc, modern 85xx #include <asm/fsl_ddr_sdram.h>, but I'm
not sure how well 83xx will receive that.  If it doesn't, I wouldn't
have a problem with taking the entire chunk as-is and putting it into
the aforementioned 83xx-protected part of common.h.

> (and special this file has a lot of ifdefs, so I wanted to prevent
>  another ifdef mess...). But I can of course move it to
> include/common.h if thats the preferred place ... ?

please.

Thanks,

Kim

Patch

diff --git a/include/mpc83xx.h b/include/mpc83xx.h
index ea137c7..e1b0929 100644
--- a/include/mpc83xx.h
+++ b/include/mpc83xx.h
@@ -1274,6 +1274,12 @@  struct pci_region;
 void mpc83xx_pci_init(int num_buses, struct pci_region **reg);
 void mpc83xx_pcislave_unlock(int bus);
 void mpc83xx_pcie_init(int num_buses, struct pci_region **reg);
+
+void disable_addr_trans(void);
+void enable_addr_trans(void);
+#if defined(CONFIG_DDR_ECC) && !defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)
+void ddr_enable_ecc(unsigned int dram_size);
+#endif
 #endif
 
 #endif	/* __MPC83XX_H__ */