Re: [RFC PATCH 05/19] powerpc: wii: bootwrapper bits

[Albert Herranz <albert_herranz@yahoo.es>]

Segher Boessenkool wrote:
>> + * 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

>> +asm ("\n\
> 
> Global asm() is evil.
> 

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

>> +    mfmsr    9\n\
>> +    andi.    0, 9, (1<<4)|(1<<5) /* MSR_DR|MSR_IR */\n\
> 
>> +    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 */

>> +    mtspr    0x01a, 8    /* SRR0 */\n\
>> +    mtspr    0x01b, 9    /* SRR1 */\n\
> 
> mtsrr0 and mtsrr1
> 

Easier :)

>> +    sync\n\
>> +    rfi\n\
> 
> No need for sync before rfi
> 

Ok.

>> +    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?

>> +    isync\n\
> 
> isync here is cargo cult
> 

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

>> +    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.

>>
> 
> 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.

>> +    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).

>> +    sync\n\
>> +    isync\n\
>> +\n\
> 
> Don't need these
> 

I'll get rid of them, then.

>> +    /* 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.
> 
>> +    /* 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.

>> +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