| Message ID | 1300690939-31511-2-git-send-email-hs@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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__ */
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(-)