[U-Boot] [PATCH v2 02/11] S3C24XX: Add core support for Samsung's S3C24XX SoCs

[Marek Vasut]

Dear José Miguel Gonçalves,

> Hi Marek,
> 
> On 14-09-2012 19:03, Marek Vasut wrote:
> > Dear José Miguel Gonçalves,
> > 
> > It's getting better :)
> 
> Hopefully :-)
> 
> > [...]
> > 
> >> +
> >> +typedef ulong(*getfreq) (void);
> > 
> > Is this used?
> 
> In the array declaration bellow...

Why, these are only values, no ?

> >> +static const getfreq freq_f[] = {
> > 
> > const array const members, no?
> 
> Do you mean I should declare it like this:
> 
> static const getfreq const freq[] = { ...

Yes

> I don't see the point because an array has no other storage besides it's
> elements. Moreover GCC generates the same object code in both ways.

Type checking, if you ever decided to write into the array, it'll prevent you 
from doing so.

> >> +	get_FCLK,
> >> +	get_HCLK,
> >> +	get_PCLK,
> >> +	get_UCLK,
> >> +};
> >> +
> >> +static const char freq_c[] = { 'F', 'H', 'P', 'U' };
> > 
> > Same here.
> > 
> >> +int print_cpuinfo(void)
> >> +{
> >> +	int i;
> >> +	char buf[32];
> >> +	ulong cpuid;
> >> +	struct s3c24xx_gpio *const gpio = s3c24xx_get_base_gpio();
> >> +
> >> +	cpuid = readl(&gpio->gstatus[1]);
> >> +	printf("CPU:  %8s (id %08lX) @ %s MHz\n", s3c24xx_get_cpu_name(),
> >> +	       cpuid, strmhz(buf, get_ARMCLK()));
> >> +	for (i = 0; i < ARRAY_SIZE(freq_f); i++)
> >> +		printf("%cCLK: %8s MHz\n", freq_c[i],
> >> +		       strmhz(buf, freq_f[i] ()));
> >> +
> >> +	return 0;
> >> +}
> > 
> > [...]
> > 
> >> +ulong get_HCLK(void)
> >> +{
> >> +	struct s3c2412_sysctl *const sysctl = s3c2412_get_base_sysctl();
> >> +	u32 clkdivn;
> >> +	u16 hclk_div, arm_div;
> >> +
> >> +	clkdivn = readl(&sysctl->clkdivn);
> >> +	hclk_div = (clkdivn & 0x3) + 1;
> >> +	arm_div = ((clkdivn >> 3) & 0x1) + 1;
> > 
> > Magic.
> 
> Missed that... I will fix it.
> 

There's more, globally please.

[...]

> >> diff --git a/include/common.h b/include/common.h
> >> index 55025c0..36f0636 100644
> >> --- a/include/common.h
> >> +++ b/include/common.h
> >> @@ -628,6 +628,7 @@ ulong	get_OPB_freq (void);
> >> 
> >>   ulong	get_PCI_freq (void);
> >>   #endif
> >>   #if defined(CONFIG_S3C24X0) || \
> >> 
> >> +    defined(CONFIG_S3C24XX) || \
> >> 
> >>       defined(CONFIG_LH7A40X) || \
> >>       defined(CONFIG_S3C6400) || \
> >>       defined(CONFIG_EP93XX)
> > 
> > What's this change? Split into different patch please.
> 
> This is related. I need this to export the functions get_FCLK(),
> get_HCLK(), get_PCLK() and get_UCLK() used in
> arch/arm/cpu/arm926ejs/s3c24xx/cpu_info.c

OK

> Best regards,
> José Gonçalves