[U-Boot] [PATCH] Introduce CONFIG_4xx_HAS_OPB

[Josh Boyer]

On Fri, 10 Oct 2008 09:33:03 +0200
Stefan Roese <sr at denx.de> wrote:

> Josh,
> 
> On Thursday 09 October 2008, Josh Boyer wrote:
> > The lib_ppc/board.c file will fill in the bi_opbfreq variable
> > in the bd_t structure for PowerPC 4xx platforms.  However, it
> > currently seems to be coupled together with the bi_pci_busfreq
> > variable under a series of ifdefs for particular CPU types.
> > As a result, it is rather easy to miss getting bi_opbfreq
> > populated on boards when doing a board port.  This is the
> > case for CONFIG_405EZ for example.
> >
> > This patch introduces a CONFIG_4xx_HAS_OPB option that is
> > set to indicate that the platform uses that variable in the
> > bd_t structure.  It decouples this from the PCI bus setting
> > and sets is properly for CPUs that have this defined.
> 
> I thought about it a little more. Some comments below...
> 
> > Signed-off-by: Josh Boyer <jwboyer at linux.vnet.ibm.com>
> >
> > diff --git a/include/asm-ppc/u-boot.h b/include/asm-ppc/u-boot.h
> > index 54ac01d..6673cd4 100644
> > --- a/include/asm-ppc/u-boot.h
> > +++ b/include/asm-ppc/u-boot.h
> > @@ -112,12 +112,14 @@ typedef struct bd_info {
> >  	unsigned char   bi_enet3addr[6];
> >  #endif
> >
> > +#if defined(CONFIG_4xx_HAS_OPB)
> > +	unsigned int	bi_opbfreq;		/* OPB clock in Hz */
> > +#endif
> 
> It just occurred to me that this change break Linux compatibility. Usually you 
> can just copy this file to the include/asm-ppc directory in Linux to have the 
> same bd_info as in U-Boot (that's old arch/ppc of course). But Linux doesn't 
> know about this CONFIG_4xx_HAS_OPB define. So this will not work.
> 
> You should probably better leave this file untouched as the 405EZ is correct.

Except the list that was originally here isn't really complete.  And as
far as Linux compatibility, it already is incompatible.  The ppcboot.h
file simply has bi_opbfreq under a #if defined(TARGET_4xx), which is
how I ran into this problem to begin with.
> 
> >  #if defined(CONFIG_405GP) || defined(CONFIG_405EP) || \
> >      defined(CONFIG_405EZ) || defined(CONFIG_440GX) || \
> >      defined(CONFIG_440EP) || defined(CONFIG_440GR) || \
> >      defined(CONFIG_440EPX) || defined(CONFIG_440GRX) || \
> >      defined(CONFIG_460EX) || defined(CONFIG_460GT)
> > -	unsigned int	bi_opbfreq;		/* OPB clock in Hz */
> >  	int		bi_iic_fast[2];		/* Use fast i2c mode */
> >  #endif
> >  #if defined(CONFIG_NX823)
> > diff --git a/include/ppc4xx.h b/include/ppc4xx.h
> > index e216663..be6696d 100644
> > --- a/include/ppc4xx.h
> > +++ b/include/ppc4xx.h
> > @@ -46,6 +46,15 @@
> >  #define CONFIG_SDRAM_PPC4xx_IBM_DDR2	/* IBM DDR(2) controller */
> >  #endif
> >
> > +/* Configure the OPB variable */
> > +#if defined(CONFIG_405GP) || defined(CONFIG_405EP) || \
> > +    defined(CONFIG_405EZ) || defined(CONFIG_440GX) || \
> > +    defined(CONFIG_440EP) || defined(CONFIG_440GR) || \
> > +    defined(CONFIG_440EPX) || defined(CONFIG_440GRX) || \
> > +    defined(CONFIG_460EX) || defined(CONFIG_460GT)
> > +#define CONFIG_4xx_HAS_OPB
> > +#endif
> 
> Hmmm. Why not something like this:
> 
> #if !defined(CONFIG_XILINX_405) && !defined(CONFIG_XILINX_440)
> #define CONFIG_4xx_HAS_OPB
> #endif
> 
> Does this work?

It would in theory, yes.  I had something like that originally, but I
did not know if boards with CPUs that weren't in the original list of
CONFIG_4nn options would break if this member suddenly showed up.  I
can't see why it would, but I have no way of testing.

josh