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

Marek Vasut
2012-09-14T18:25:25Z

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