Re: [U-Boot] [PATCH 3/6] arm: uniphier: add UniPhier SoC suppurt code

21 Aug 2014

      Hi Albert,
First, I don't like full-quoting as Wolfgang also mentioned before:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/52214/focus=52252
Since this patch is very long, I might miss some of your comments.
If I did not overlook, my comments are below:
On Wed, 20 Aug 2014 11:31:26 +0200
Albert ARIBAUD albert.u.boot@aribaud.net wrote:
...
Hi Masahiro,
On Fri,  4 Jul 2014 19:19:15 +0900, Masahiro Yamada
yamada.m@jp.panasonic.com wrote:
...
Subject: [PATCH 3/6] arm: uniphier: add UniPhier SoC suppurt code
Typo in subject -- since you're going to do a v2, please fix this too,
thanks!
Oops, thanks for pointing this out.
Will fix in v2.
I am still holding v2 back because of the patch dependency.
...
...
+}

+int board_late_init(void)
+{

puts("MODE:  ");

switch (spl_boot_device()) {
case BOOT_DEVICE_NAND:
printf("NAND Boot\n");

setenv("bootmode", "nandboot");

nand_denali_wp_disable();

break;

default:
printf("Unsupported Boot Mode\n");

Please add the mode value (decimal, hex or binary as you prefer, as long
as the reader can understand the format) to the output.
BOOT_DEVICE_* is defined by enum in arch/arm/include/spl.h.
I don't see the necessity to display the meaningless value.
...
...

SSC_RANGE_OP_MAX_SIZE : size;

__uniphier_cache_maint_range(start, chunk_size,

operation); +

start += chunk_size;

size -= chunk_size;

}

writel(SSCOPE_CM_SYNC, SSCOPE); /* drain internal buffers */
readl(SSCOPE); /* need a read back to confirm */

Does the readback contain some status showing whether the drain is in
progress vs. complete? If it does, then shouldn't the line above be a
loop waiting for completion?
Not really.
I am not familiar enough with this cache block to tell you its detail
but the hardware manual clearly says to read back SSCOPE register
so I am following it.
...
...

+int print_cpuinfo(void)
+{

u32 revision, type, model, rev, required_model = 1,

required_rev = 1; +

revision = readl(SG_REVISION);
type = (revision & SG_REVISION_TYPE_MASK) >>

SG_REVISION_TYPE_SHIFT;

model = (revision & SG_REVISION_MODEL_MASK) >>

SG_REVISION_MODEL_SHIFT;

rev = (revision & SG_REVISION_REV_MASK) >>

SG_REVISION_REV_SHIFT; +

puts("CPU:   ");

switch (type) {
case 0x25:
puts("PH1-sLD3 (MN2WS0220)");

required_model = 2;

break;

case 0x26:
puts("PH1-LD4 (MN2WS0250)");

required_rev = 2;

break;

case 0x28:
puts("PH1-Pro4 (MN2WS0230)");

break;

case 0x29:
puts("PH1-sLD8 (MN2WS0270)");

break;

default:
printf("Unknown Processor ID (0x%x)\n", revision);

return -1;

}

Maybe use symbolic constants rather tha magic numbers in switch cases
above.
Do you want me to do like this?
#define PH1_SLD3_ID	0x25
#define PH1_LD4_ID	0x26
#define PH1_PRO4_ID	0x28
#define PH1_SLD8_ID	0x29
switch (type) {
    case PH1_SLD3_ID:
    	puts("PH1-sLD3 (MN2WS0220)");
    	required_model = 2;
    	break;
    case PH1_LD4_ID:
    	puts("PH1-LD4 (MN2WS0250)");
    	required_rev = 2;
    	break;
    case PH1_PRO4_ID:
    	puts("PH1-Pro4 (MN2WS0230)");
    	break;
    case PH1_SLD8_ID:
    	puts("PH1-sLD8 (MN2WS0270)");
    	break;
    default:
    	printf("Unknown Processor ID (0x%x)\n", revision);
    	return -1;
    }
I am not sure if we should be strict to the magic number rule here
in this code.
It is true 0x25 is a magic number but
its meaning is apparent enough at a glance.
...
...

TBL_ENTRY(0xffc), TBL_ENTRY(0xffd), TBL_ENTRY(0xffe),

TBL_ENTRY(0xfff), +};
This table seems rather predictable -- the value of each entry seems to
be computable from the index alone. Is there a shorter way to define
it? I mean, here, it is an initialized non-const, bu we could also make
it a un-initialized non-const and fill it through a for-loop, could we
not?
I wish I could, but it is impossible.
UnPhier SoCs have no dedicated SRAM; it is shared with L2 Cache.
To turn on L2 Cache, this page table is necessary.
At the time, no writable memory is available yet.
That is why this table must be computed at the compilation
and placed in the .rodata section.
Indeed, this is an unfortunate hardware specification.
...
...

+int board_postclk_init(void)
+{

bcu_init();

sbc_init();

sg_init();

pll_init();

uniphier_board_init();

led_write(B, 1, , );

pin_init();

led_write(B, 2, , );

clkrst_init();

led_write(B, 3, , );

return 0;

Can you remove the blank lines? Ditto in other places where double
spacing occurs.
Assume you mentioned led_write(B, 1, , ).
This is not a function but a macro.
It is impossible to remove the blank arguments.
Best Regards
Masahiro Yamada