[U-Boot] [PATCH 2/2] MPC8308ERDB: minimal support for devboard from Freescale

[Ilya Yanok <yanok@emcraft.com>]

Hi Kim,

Thanks for your review.

On 23.06.2010 04:17, Kim Phillips wrote:
>>   MAKEALL                                   |    1 +
>>   Makefile                                  |    3 +
>>   board/freescale/mpc8308erdb/Makefile      |   52 +++
>>   board/freescale/mpc8308erdb/config.mk     |    1 +
>>   board/freescale/mpc8308erdb/mpc8308erdb.c |  154 ++++++++
>>   board/freescale/mpc8308erdb/sdram.c       |  126 +++++++
>>   include/configs/MPC8308ERDB.h             |  572 +++++++++++++++++++++++++++++
>>   7 files changed, 909 insertions(+), 0 deletions(-)
>>   create mode 100644 board/freescale/mpc8308erdb/Makefile
>>   create mode 100644 board/freescale/mpc8308erdb/config.mk
>>   create mode 100644 board/freescale/mpc8308erdb/mpc8308erdb.c
>>   create mode 100644 board/freescale/mpc8308erdb/sdram.c
>>   create mode 100644 include/configs/MPC8308ERDB.h
>>      
> missing a doc/README.mpc8308erdb file.
>    

Hm.. Wolfgang, do we really need this?
Well, I hope I'd be able to reuse most of doc/README.mpc8315erdb...

>>
>> +MPC8308ERDB_config: unconfig
>> +	@$(MKCONFIG) -a MPC8308ERDB powerpc mpc83xx mpc8308erdb freescale
>> +
>>      
> patch doesn't apply to WD's next branch:
>
> ---
> Applying: MPC8308ERDB: minimal support for devboard from Freescale
> error: patch failed: Makefile:2233
> error: Makefile: patch does not apply
> /home/r1aaha/git/u-boot/.git/rebase-apply/patch:112: new blank line at EOF.
> +
> Patch failed at 0002 MPC8308ERDB: minimal support for devboard from Freescale
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
> ---
>
> please rebase on top of Wolfgang's next branch, and also fix the new
> blank line at EOF.
>    

Yes, I've already done this.

>> +#define CONFIG_MPC831x		1 /* MPC831x CPU family */
>>      
> I don't see the 8308 being a member of the 831x family; e.g., upper
> spridr bits are 0x810, which don't match those of 831x: 0x80B.
> Please introduce a new CONFIG_MPC830x.
>    

Actually I've noticed this myself... But 8308 has many things common 
with 831x and I'm not sure if there is a family of 830x (I can see only 
8308 on Freescale website).

Well, I'll do as you propose.

>> +/*
>> + * There are neither HRCWH_PCI_HOST nor HRCWH_PCI1_ARBITER_ENABLE bits
>> + * in 8308's HRCWH according to the manual, but original Freescale's
>> + * code has them and I've expirienced some problems using the board
>> + * with BDI3000 attached when I've tried to set these bits to zero
>> + * (UART doesn't work after the 'reset run' command).
>>      
> is the BDI overriding the RCW in this case?  it's not clear..
>    

Yes, my default BDI configuration overrides RCW with 0xa0606c00 
0x44060000 (with HRCWH_PCI_HOST and HRCWH_PCI1_ARBITER_ENABLE bits set) 
to be able to program empty flash.
My test sequence is as follows:
0. I had BDI overriding RCW and programmed U-Boot with the same RCW values.
1. I compiled new U-Boot image without HRCWH_PCI* bits and reflashed it 
from U-Boot itself.
2. Then I rebooted the board with the U-Boot 'reset' command. Everything 
works fine.
3. Reseted the board with 'reset run' command from the BDI. Everything 
works well too (recall that BDI is configured to override RCW with old 
value).
4. Then I've changed BDI configuration disabling the RCW override. 
Everything works.
5. But if I configure BDI to override RCW with new value (without 
HRCWH_PCI* bits) I can't see anything on serial console after reset (but 
it looks like U-Boot is started correctly, at least execution address 
looks correct).

>> +#define CFG_SICRH		0x01001003
>> +#define CFG_SICRL		0x00000040 /* 3.3V, no delay */
>>      
> the CFG_ prefix is no longer - are these even being used anywhere?
>    

Ouch... That's my bug. I'll fix it.

>> +#define CONFIG_SYS_HID0_FINAL	(HID0_ENABLE_MACHINE_CHECK | \
>> +				 HID0_ENABLE_DYNAMIC_POWER_MANAGMENT)
>>      
> | HID0_ENABLE_INSTRUCTION_CACHE?
>    

Yes, you are right. I'll add it.

Regards, Ilya.