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

[Yuli Barcohen]

>>>>> Wolfgang Denk writes:

    Wolfgang> In message <16469.31569.535909.246506 at gargle.gargle.HOWL>
    Wolfgang> you wrote:

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

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

    Wolfgang> I don't know. I just want to understand the consequences.

    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> First, ther is at least one redundand
    Wolfgang> "gd->CPM_DATAONLY_SIZE;" here.

No, no, it's just E-mail formatting of the excerpt after several
replies. There is no redundant code in the patch.

    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.

    Yuli> There should be some misunderstanding
    Yuli> here. PQ27E_DATAONLY_SIZE is NOT board-specific (I won't add
    Yuli> board-specific things into common files). PQ27e is the common
    Yuli> name for the 8247/8248/8271/8272 (the `e'

    Wolfgang> Sorry for chosing a poor description.

    Yuli> stands for `encryption'). These chips have got much less
    Yuli> internal memory (DPRAM) and at different addresses so they
    Yuli> must have separate
    Yuli> #defines. CPM_DATAONLY_SIZE is just incorrect for PQ27e. PQ27e
    Yuli> have got only 4K of "data only" RAM and not 8K (and at
    Yuli> different base, BTW).

    Wolfgang> Why do you need a separate #define then?  Isn't it
    Wolfgang> sufficient to
    Wolfgang> #define a correct value for CPM_DATAONLY_SIZE then?

This would require that, for every PQ27e-based board, it would be
defined in the board configuration file that it's PQ27e and not just
8260. Then CPM_DATAONLY_SIZE can be defined conditionally on this
definition in cpm_8260.h. I personally don't like such compile-time
definitions because chip version can be easily detected in run-time.

    Wolfgang> PQ27E_FCC_SPECIAL_BASE vs. CPM_FCC_SPECIAL_BASE

    Yuli> It's for the same reason. PQ27e have not got RAM at 0xB000,
    Yuli> other PQ2s

    Wolfgang> So why not just #define a correct value?

    Yuli> After the explanations, what changes would you suggest?

    Wolfgang> Use the existing variables and just assign correct values?

Well, the question is if it's better that the CPU-specific code will
silently handle the differences or we'll make it the user's
responsibility to define the difference manually. I think automatic
handling is better but if you insist on #ifdefs, I can implement it too.

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