[U-Boot] RFC [PATCH 2/5] arm/kirkwood: print speeds with cpu info.

[Prafulla Wadaskar]

> -----Original Message-----
> From: Jason [mailto:u-boot at lakedaemon.net]
> Sent: Thursday, July 28, 2011 7:02 AM
> To: Prafulla Wadaskar
> Cc: clint at debian.org; wd at denx.de; u-boot at lists.denx.de; Prabhanjan
> Sarnaik; Ashish Karkare; Siddarth Gore; bdale at gag.com
> Subject: Re: RFC [PATCH 2/5] arm/kirkwood: print speeds with cpu info.
> 
> On Wed, Jul 27, 2011 at 11:21:16AM -0700, Prafulla Wadaskar wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jason Cooper [mailto:u-boot at lakedaemon.net]
> > > Sent: Wednesday, July 27, 2011 2:49 AM
> > > To: clint at debian.org; wd at denx.de; Prafulla Wadaskar
> > > Cc: u-boot at lists.denx.de; Prabhanjan Sarnaik; Ashish Karkare;
> Siddarth
> > > Gore; bdale at gag.com; Jason Cooper
> > > Subject: RFC [PATCH 2/5] arm/kirkwood: print speeds with cpu info.
> > >
> > >
> > > Signed-off-by: Jason Cooper <u-boot at lakedaemon.net>
> > > ---
> > >  arch/arm/cpu/arm926ejs/kirkwood/cpu.c    |   46
> > > ++++++++++++++++++++++++++++++
> > >  arch/arm/include/asm/arch-kirkwood/cpu.h |    1 +
> > >  2 files changed, 47 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/arch/arm/cpu/arm926ejs/kirkwood/cpu.c
> > > b/arch/arm/cpu/arm926ejs/kirkwood/cpu.c
> > > index b4a4c04..a69f9f2 100644
> > > --- a/arch/arm/cpu/arm926ejs/kirkwood/cpu.c
> > > +++ b/arch/arm/cpu/arm926ejs/kirkwood/cpu.c
> > > @@ -270,11 +270,26 @@ static void kw_sysrst_check(void)
> > >  }
> > >
> > >  #if defined(CONFIG_DISPLAY_CPUINFO)
> > > +#define MSAR_CPUCLCK_EXTRACT(X)	(((X & 0x2) >> 1) | ((X &
> 0x400000) >>
> > > 21) | \
> > > +				((X & 0x18) >> 1))
> > > +#define MSAR_L2CLCK_EXTRACT(X)  (((X & 0x600) >> 9) | ((X &
> 0x80000) >>
> > > 17))
> > > +#define MSAR_DDRCLCK_RTIO_MASK  (0xf << 5)
> > > +
> > > +#define MSAR_TCLCK_OFFS		21
> > > +#define MSAR_TCLCK_MASK		(0x1 << MSAR_TCLCK_OFFS)
> > > +#define MV_BOARD_TCLK_166MHZ	166666667
> > > +#define MV_BOARD_TCLK_200MHZ	200000000
> > > +#define MSAR_TCLCK_167		(0x1 << MSAR_TCLCK_OFFS)
> > > +#define MSAR_TCLCK_200		(0x0 << MSAR_TCLCK_OFFS)
> >
> > There are one time used macros, can they be optimized?
> 
> Sure, do you prefer a static assignment:
> 
> #define MSAR_TCLCK_166 	0x00200000


This is okay with some comments.

> 
> or, just do away with it altogether and use the number?
> 
> > > +
> > >  int print_cpuinfo(void)
> > >  {
> > >  	char *rev;
> > >  	u16 devid = (readl(KW_REG_PCIE_DEVID) >> 16) & 0xffff;
> > >  	u8 revid = readl(KW_REG_PCIE_REVID) & 0xff;
> > > +	u32 cpu_clk, t_clk, tmp;
> > > +	u32 sys_clk, l2_clk;
> > > +	u32 l2_ratio, ddr_ratio;
> > >
> > >  	if ((readl(KW_REG_DEVICE_ID) & 0x03) > 2) {
> > >  		printf("Error.. %s:Unsupported Kirkwood SoC 88F%04x\n",
> > > __FUNCTION__, devid);
> > > @@ -297,6 +312,37 @@ int print_cpuinfo(void)
> > >  	}
> > >
> > >  	printf("SoC:   Kirkwood 88F%04x_%s\n", devid, rev);
> > > +
> > > +	tmp = readl(MPP_SAMPLE_AT_RESET);
> > > +	cpu_clk = MSAR_CPUCLCK_EXTRACT(tmp);
> > > +	if (cpu_clk == 0x9)
> > > +		cpu_clk = 1200;
> > > +
> > > +	l2_ratio = MSAR_L2CLCK_EXTRACT(tmp);
> > > +	l2_clk = cpu_clk / l2_ratio;
> > > +
> > > +	ddr_ratio = tmp & MSAR_DDRCLCK_RTIO_MASK;
> > > +	ddr_ratio = ddr_ratio >> 5;
> > > +	if (ddr_ratio == 4)
> > > +		sys_clk = 400;
> > > +
> > > +	switch (tmp & MSAR_TCLCK_MASK) {
> > > +	case MSAR_TCLCK_167:
> > > +		t_clk = MV_BOARD_TCLK_166MHZ;
> > > +		break;
> > > +	case MSAR_TCLCK_200:
> > > +		t_clk = MV_BOARD_TCLK_200MHZ;
> > > +		break;
> > > +	default:
> > > +		t_clk = MV_BOARD_TCLK_200MHZ;
> > > +		break;
> > > +	}
> > > +
> > > +	printf("CPU running @ %dMHz L2 running @ %dMHz\n",
> > > +		cpu_clk, l2_clk);
> > > +	printf("SysClock = %dMHz, TClock = %dMHz\n",
> > > +		sys_clk, t_clk / 1000000);
> >
> > It is good to encapsulate this in #ifdef CONFIG_SYS_XXX
> 
> It's already encapsulated in CONFIG_DISPLAY_CPUINFO, so if enabled, on
> boot it looks like:
> 
> SoC:   Kirkwood 88F6281_A1
> CPU running @ 1200MHz L2 running @ 400MHz
> SysClock = 400MHz, TClock = 200MHz
> 
> I can encapsulate it separately if you want, but it seems a little too
> fine grained for me.  Either I want cpu info, or I don't. ymmv.

I think you are right, this should be okay, anyway it displays useful information.
Sorry for this noisy comments.

Regards..
Prafulla . .