Segher Boessenkool wrote:

quoted

+ * We enter with an unknown cache, high BATs and MMU status.

What does this mean?  You know the low four BATs on entry and
nothing else?

That means that we do not make assumptions regarding:
- the state of the cache (enabled vs disabled)
- if the high BATs are enabled or not
- if the MMU is enabled or not

Is this clearer? I'm not a native english speaker as you may have noticed already :-P

quoted

+asm ("\n\

Global asm() is evil.

Yes, you said. Still, I'd like a proper argument :)

quoted

+    mfmsr    9\n\
+    andi.    0, 9, (1<<4)|(1<<5) /* MSR_DR|MSR_IR */\n\

quoted

+    andc    9, 9, 0\n\

mfmsr 9 ; rlwinm 9,9,0,~0x30 ?

Yes. Maybe

mfmsr 9 ; rlwinm 9,9,0,~((1<<4)|(1<<5)) /* MSR_DR|MSR_IR */

quoted

+    mtspr    0x01a, 8    /* SRR0 */\n\
+    mtspr    0x01b, 9    /* SRR1 */\n\

mtsrr0 and mtsrr1

Easier :)

quoted

+    sync\n\
+    rfi\n\

No need for sync before rfi

Ok.

quoted

+    mtspr    0x210, 8    /* IBAT0U */\n\
+    mtspr    0x211, 8    /* IBAT0L */\n\

You only need to set the upper BAT to zero, saves some code.

Great. Is this documented somewhere?

quoted

+    isync\n\

isync here is cargo cult

I'll offer a dead chicken to compensate for this.

quoted

+    li    8, 0x01ff    /* first 16MiB */\n\
+    li    9, 0x0002    /* rw */\n\
+    mtspr    0x210, 8    /* IBAT0U */\n\
+    mtspr    0x211, 9    /* IBAT0L */\n\
+    mtspr    0x218, 8    /* DBAT0U */\n\
+    mtspr    0x219, 9    /* DBAT0L */\n\

M=0 for RAM?

See analog question for gamecube bootwrapper bits.

quoted

Also, you should normally write the lower BAT first.  Doesn't matter
here because IR=DR=0 of course.

I can change that too if it's the general way to go.

quoted

+    lis    8, 0xcc00    /* I/O mem */\n\
+    ori    8, 8, 0x3ff    /* 32MiB */\n\
+    lis    9, 0x0c00\n\
+    ori    9, 9, 0x002a    /* uncached, guarded, rw */\n\
+    mtspr    0x21a, 8    /* DBAT1U */\n\
+    mtspr    0x21b, 9    /* DBAT1L */\n\

Is there any real reason you don't identity map this?

No. There's a bit of nostalgia there.
(On the other hand there's no real reason to identity map it).

quoted

+    sync\n\
+    isync\n\
+\n\

Don't need these

I'll get rid of them, then.

quoted

+    /* enable high BATs */\n\
+    lis    8, 0x8200\n\
+    mtspr    0x3f3, 8    /* HID4 */\n\

You need to use read-modify-write here.  Also, shouldn't you
enable the extra BATs before setting them?

No. You should first set them up and then enable them.
The content of the HIGH BATs is undefined on boot, AFAIK.

And you _do_ need isync here as far as I can see.

quoted

+    /* enable caches */\n\
+    mfspr    8, 0x3f0\n\
+    ori    8, 8, 0xc000\n\
+    mtspr    0x3f0, 8    /* HID0 */\n\
+    isync\n\

You need to invalidate the L1 caches at the same time as you enable
them.

Ok. I'll check if the caches are enabled. If they aren't I'll invalidate and enable them.

quoted

+void platform_init(unsigned long r3, unsigned long r4, unsigned long r5)
+{
+    u32 heapsize = 24*1024*1024 - (u32)_end;
+
+    simple_alloc_init(_end, heapsize, 32, 64);
+    fdt_init(_dtb_start);
+
+    if (!ug_grab_io_base() && ug_is_adapter_present())

The "!" reads weird.  Can you not make ug_is_adapter_present()
call ug_grab_io_base(), anyway?

Calling ug_grab_io_base() from ug_is_adapter_present() can be misleading too (you are supposed to just check if the adapter is present in that function, according to the name).
If the "!" is ugly I can use the following idiom, introducing an error variable.

  error = ug_grab_io_base();
  if (!error && ug_is_adapter_present())
     /* blah */

Thanks,
Albert