[U-Boot-Users] Re: Patch: Support for PQ27e (8247/48/71/72) chips and MPC8272ADS board

[Yuli Barcohen]

>>>>> Wolfgang Denk writes:

    Wolfgang> Dear Yuli, in message
    Wolfgang> <16450.14241.889562.784232 at gargle.gargle.HOWL> you wrote:

    Yuli> This patch adds support for the newest additions to Motorola
    Yuli> PQII family (MPC8247/8248/8271/8272) known also as
    Yuli> PQ27e. They've got different DPRAM layout (as was discussed on
    Yuli> the list) so common files were changed. I tested the code on
    Yuli> several boards I've got (using both old and new
    Yuli> chips). Motorola released new derivative of MPC8260ADS
    Yuli> evaluation board supporting these chips: MPC8272ADS. This
    Yuli> patch adds support for the new board.

    Wolfgang> Thanks.

    Wolfgang> Please don't forget the entry for the CHANGELOG file.

Sorry, I promise it was last time I forgot it.

    Wolfgang> Please add the appropriate additions to the CREDITS and
    Wolfgang> MAINTAINERS files, if needed.

    Wolfgang> I don't like the following parts of your patch:

    Wolfgang> 	Index: cpu/mpc8260/commproc.c
    Wolfgang> 	========================================
    Wolfgang> 	RCS file:
    Wolfgang> 	/home/CVS/u-boot/u-boot/cpu/mpc8260/commproc.c,v
    Wolfgang> 	retrieving revision 1.1.1.3 diff -p -u -r1.1.1.3
    Wolfgang> 	commproc.c
    Wolfgang> --- cpu/mpc8260/commproc.c 6 Aug 2003 17:03:01 -0000
    Wolfgang> 	    1.1.1.3
    Wolfgang> +++ cpu/mpc8260/commproc.c 29 Feb 2004 18:45:04 -0000
    Wolfgang> 	@@ -20,13 +20,6 @@
    Wolfgang> 	 #include <common.h> include <asm/cpm_8260.h>
	 
    Wolfgang> -/*
    Wolfgang> - * because we have stack and init data in dual port ram
    Wolfgang> - * we must reduce the size
    Wolfgang> - */
    Wolfgang> -#undef CPM_DATAONLY_SIZE
    Wolfgang> -#define CPM_DATAONLY_SIZE ((uint)(8 * 1024) - CPM_DATAONLY_BASE)

    Wolfgang> Are you 100% sure this does not break any existing boards?

I checked it on four different boards with different PQ2 chips. In fact,
for older (pre-PQ27e) chips this patch does not change memory map so
they aren't affected in any way. Am I missing anything?

    Wolfgang> 	 void m8260_cpm_reset(void) {
    Wolfgang> 	@@ -38,7 +31,10 @@ m8260_cpm_reset(void)
    Wolfgang> /* Reclaim the DP memory for our use.  */
    Wolfgang> gd-> dp_alloc_base = CPM_DATAONLY_BASE;
    Wolfgang> - gd->dp_alloc_top = gd->dp_alloc_base
    Wolfgang> + gd->CPM_DATAONLY_SIZE;
    Wolfgang> + if (is_pq27e())
    Wolfgang> + gd->dp_alloc_top = gd->dp_alloc_base
    Wolfgang> + gd->PQ27E_DATAONLY_SIZE;
    Wolfgang> + else
    Wolfgang> + gd->dp_alloc_top = gd->dp_alloc_base
    Wolfgang> + gd->CPM_DATAONLY_SIZE;
	 
    Wolfgang> /* Reset CPM

    Wolfgang> Please do not add a board-specific #define
    Wolfgang> PQ27E_DATAONLY_SIZE when we already have a
    Wolfgang> CPM_DATAONLY_SIZE which could be used.

There should be some misunderstanding here. PQ27E_DATAONLY_SIZE is NOT
board-specific (I won't add board-specific things into common
files). PQ27e is the common name for the 8247/8248/8271/8272 (the `e'
stands for `encryption'). These chips have got much less internal memory
(DPRAM) and at different addresses so they must have separate
#defines. CPM_DATAONLY_SIZE is just incorrect for PQ27e. PQ27e have got
only 4K of "data only" RAM and not 8K (and at different base, BTW).

    Wolfgang> The same happens again in other places, like:

    Wolfgang> cpu/mpc8260/ether_fcc.c:

    Wolfgang> 	PQ27E_FCC_SPECIAL_BASE vs. CPM_FCC_SPECIAL_BASE

It's for the same reason. PQ27e have not got RAM at 0xB000, other PQ2s
have not got RAM at 0x9000. There is simply no common base for the two
families so I had to #define new constants. Any suggestions for better
solution are welcome.

    Wolfgang> Please clean up and resubmit.

After the explanations, what changes would you suggest?

-- 
========================================================================
 Yuli Barcohen       | Phone +972-9-765-1788 |  Software Project Leader
 yuli at arabellasw.com | Fax   +972-9-765-7494 | Arabella Software, Israel
========================================================================