On Thu, 27 Sep 2012 09:21:19 +0200
Gerlando Falauto <gerlando.fala...@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.fala...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
