[U-Boot] [PATCH v1 2/4] mpc83xx: add support for mpc8309

[Kim Phillips]

On Thu, 27 Sep 2012 09:21:19 +0200
Gerlando Falauto <gerlando.falauto at keymile.com> wrote:

> On 09/27/2012 03:22 AM, Kim Phillips wrote:
> > On Wed, 26 Sep 2012 10:28:08 +0200
> > Gerlando Falauto<gerlando.falauto at keymile.com>  wrote:
> >
> >>   - CONFIG_MPC83XX_FEAT_QE
> >
> > this could be CONFIG_QE but should probably be CONFIG_HAS_FSL_QE
> > which doesn't exist.
> 
> Assuming we keep CONFIG_QE, do you think that could replace the whole:
> 
> #if defined(CONFIG_MPC8309) || defined(CONFIG_MPC8360) || 
> defined(CONFIG_MPC832x)
> 
> which I am not very happy with?

sure, since that's what's protecting qe_clk in asm/global_data.h.

> >> @@ -120,14 +122,17 @@ int get_clocks(void)
> >>   #if defined(CONFIG_FSL_ESDHC)
> >>   	u32 sdhc_clk;
> >>   #endif
> >> +#if !defined(CONFIG_MPC8309)
> >>   	u32 enc_clk;
> >> +#endif
> >
> > the 8309 is supposed to be similar to the 8308, which also doesn't
> > have enc_clk (even though it doesn't do this).  I'm thinking
> > CONFIG_MPC8308 should be renamed _MPC830x before adding support for
> > the 8309.
> 
> Wouldn't that be confusing? The way I understand it we'd also need some 
> way to distinguish between the two, so we'd have:
> 
> #define CONFIG_MPC83xx          1
> #define CONFIG_MPC830x		1
> #define CONFIG_MPC8309          1

yes, I didn't mean all 8308 be renamed, only the relevant ones here.

> Plus (assuming my patch is functionally correct), there's only a couple 
> of occurences of:
> 
> #if defined(CONFIG_MPC8308) || defined(CONFIG_MPC8309)

so far the 8308 and the 8360 are the only SoCs that don't have a
'tens'/MPC83Xx defines.  The 8360 can get away with it, since it's
very close to the 8358 (and we can avoid that marketing-imposed
snafu).  The 8308 and 9 are more different (QE vs TSEC), but as seen
here, there are other commonalities.

> >> @@ -457,6 +470,8 @@ int get_clocks(void)
> >>   	gd->tsec1_clk = tsec1_clk;
> >>   	gd->tsec2_clk = tsec2_clk;
> >>   	gd->usbdr_clk = usbdr_clk;
> >> +#elif defined(CONFIG_MPC8309)
> >> +	gd->usbdr_clk = usbdr_clk;
> >>   #endif
> >
> > this change generates this new compiler warning:
> >
> > Configuring for MPC8308RDB board...
> >     text	   data	    bss	    dec	    hex	filename
> >   261821	   6860	 235952	 504633	  7b339	./u-boot
> > speed.c: In function 'get_clocks':
> > speed.c:472:16: warning: 'usbdr_clk' may be used uninitialized in this function [-Wuninitialized]
> 
> Actually it's this one:
> 
> @@ -185,7 +190,10 @@ int get_clocks(void)
>                  /* unkown SCCR_TSEC1CM value */
>                  return -2;
>          }
> +#endif
> 
> +#if defined(CONFIG_MPC8309) || defined(CONFIG_MPC831x) || \
> +       defined(CONFIG_MPC834x) || defined(CONFIG_MPC837x)
>          switch ((sccr & SCCR_USBDRCM) >> SCCR_USBDRCM_SHIFT) {
>          case 0:
>                  usbdr_clk = 0;
> 
> where the code gets dropped in the case of 8308.
> So, do you think CONFIG_HAS_FSL_DR_USB would do the trick in that case?

now that I take a closer look, not all 831/4/7x boards set
HAS_FSL_DR_USB, and I'm afraid that might break something.

But I also think MPC830x would do the trick nicely, since it'd be
a step toward 8308 usb enablement.

> >> +#define SICR_2_QUIESCE_B		(0<<  (30-24))
> >> +
> >>   #endif
> >
> > was there an inadequacy in the other SoCs' SICRL/H_ naming
> > convention and/or value definition in this area?  If not, then why
> > should the 8309 get its own reinvented SICR_1/2_ etc.?
> 
> As for the naming, I used SICR_1/2 as opposed to SICRL/H because that's 
> how the registers are called in the datasheet.

Hadn't realized that.  Wunderbar.  I suppose documentation people
don't need to have the same sense of consistency we do.

> As for the value definition, I added my own (third, at least!) 
> convention so to match the bit numbering in the datasheet.
> This should makes double checking a trivial task.

I suppose, as I say:

>  > Just looking for some consistency here...

but, ok - SICRs aren't used that much and we're at the end of the
83xx line.

> Thanks a lot for your review!

Cheers,

Kim