[U-Boot] [PATCH] powerpc: add support for the Freescale P1022DS reference board

[Timur Tabi]

Wolfgang Denk wrote:

> fsl_ddr_sdram() does NOT the same as get_ram_size(). It does not
> perform any actual testing, and will not detect errors in the SPD
> information or in the code processing it.

Ok, I'll add it.

>> This code is identical to the code in the p2020ds.c, so I'm just
>> mirroring it.  The only difference is the names of the slots in the
>> printf.  I would prefer to keep this new code as close as possible to
>> our existing code.  I suspect that we'll be unifying the PCI code in
>> the future, and keeping it similar will make it easier.
> 
> Thanks for pointing this out. so we have even more occurrences of this
> block of code - time has come to factor this out into a common
> function that is used for this board and for p2020ds as well.

Kumar just posted some patches for p2020ds that changes this, so I'll
incorporate those.

>>> This CONFIG_GET_CLK_FROM_ICS307 is documented?
>>
>> We've been using it for years.  Now you complain?  It's the same code
>> in almost all of our recent boards.
> 
> Please document it.

Looks like Kumar posted patches for this, too, so I'll add those as well.

> I complain about things when I see them.
> 
> Do you really expect me to review all patches submitted on this list
> in scrupulous detail? 

No, apparently just mine.

> It would be nice if you also fixed all the other dozen places where
> such code has crept in.

Can I do one thing at a time?  I agree that a lot of those could be
optimized, and if you look at the git log for our stuff, you'll see we've
been doing that a lot in the past year.  But in general, fixes like these
usually work better if the board support is already present in your repository.

>>> PCIE1 => pci1 => pcie1_hose
>>> PCIE2 => pci0 => pcie2_hose
>>> PCIE3 => pci2 => pcie3_hose
>>>
>>> Weird, weird...
>>
>> I asked Kumar about it, but he didn't really have much to say, so I left it.
> 
> You mean neither of you knows if this is correct or wrong ???

I guess I should clarify.  Kumar said it was based on the order of the
addresses of the PCI devices within CCSR space, but he didn't offer any
insight as to whether it should be fixed or how.

>>>> +++ b/board/freescale/p1022ds/p1022ds_diu.c
>>>> @@ -0,0 +1,151 @@
>>>
>>> This should probably go to drivers/video/ ?
>>
>> It's p1022-specific, so I don't see why.
> 
> No, it's not. It is largely the same as mpc8610hpcd_diu.c; common
> video driver code belongs into drivers/video/

Ok, will you allow me to merge those two functions into a single driver at a
later time, when I have the opportunity to examine the code and test it on
both boards?

>>>> +#ifndef __ASSEMBLY__
>>>> +extern unsigned long calculate_board_sys_clk(void);
>>>> +extern unsigned long calculate_board_ddr_clk(void);
>>>> +#endif
>>>
>>> Please move to appropriate header file.
>>
>> OMG, every single one of our header files does this!!!!  We've been
>> doing this for years!
>>
>> I really do believe you're picking on me now.
> 
> I'm not. I'm just reviewing a patch.
> 
> It would be great if you cleaned up the other places as well.

Well, this particular change would probably need to be made later, because I
would probably need to change all the board header files at once.  It's not
trivial.

-- 
Timur Tabi
Linux kernel developer at Freescale