[U-Boot] [PATCH] ppc44x: RFC: Unification of virtex5 pp440 boards

Michal Simek monstr at seznam.cz
Tue Aug 26 16:32:18 CEST 2008


And one more thing. I would like to see readme in one board folder where will be
written what design it is. If you use reference design from xilinx page please
write it. Or if is the design from BSB just write it to this file.

M

Ricardo Ribalda Delgado wrote:
> Hi Stefan
> 
>   First of all: Thanks for taking some time in reading the patch
> 
> 
>> I looked at your patch.
>> your xparameters contain space before tab
> 
> Sorry :), BTW: do you know how to highlight this in vim?
> 
>> macro XPAR_DDR2_SDRAM_MEM_BASEADDR is in generic file
>> board/xilinx/ppc440-generic/xilinx_ppc440_generic.c. This is not much generic
>> value. MB use XILINX_RAM_START for every memory. Software doesn't care if is
>> ddr2 or ddr or sdram - just memory. That was a reason why I did special bsp for
>> generation of xparameters.h.
>>
> 
> I would like to mantain the Xparameters file created by edk: It is
> very convenient for the user, he just have to replace the
> xparameters.h file with his file.
> 
> I can add something like
> #ifdef XPAR_OTHER_RAM_NAME
>   #define #XPAR_DDR2_SDRAM_MEM_BASEADDR XPAR_OTHER_RAM_NAME
> #endif
> 
>> avnet and ml507 folder contain only checkboard function with one puts. This is
>> not much for whole board. The name should be in xparameters.h and written in
>> generic platform.
> 
> ml507 and avnet directories was just an example of how to create a
> board that replaces the generic functions. Anyway, I consider to left
> them, those two are the more representative boards and should be
> supported explicitly by u-boot. Also, there is no code replication
> 
>> I see simplier way just add
>> #define BOARD_NAME "Avnet bla bla" or for xilinx too. And small change in puts.
>>
> 
> If the user is using the generic design, he should see generic design
> in the prompt. Anyway, this is just a personal opinion, no big deal
> changing this
> 
> 
>> Why is xparameters.h in generic folder? You meant that is generic code - there
>> should be nothing specific like addresses are. (That's for your patches not for
>> generic solution which I have in my mind)
> 
> The generic folder is also a the generic-board folder.
> 
>> XPAR_SPI_0_NUM_TRANSFER_BITS, XPAR_IIC_EEPROM_BASEADDR and XPAR_SPI_0_BASEADDR
>> are not used anywhere. And maybe some others. (LL_TEMAC driver is not in U-BOOT
>> -> I have full version with fifo and sgdma in my repo but LLTEMAC_0_BASEADDR is
>> not used in U-BOOT yet)
>>
> 
> I use it on my design, that lines can be replaced. Anyway I have a
> working version of Xilinx SPI, I am waiting for some more tests to
> release it.
> 
> 
>> Please compare both configs files - you can see changes which are board specific.
>> 1. point to proper xparameters.h
>> 2. ENV_OFFSET -> this could be generic
> 
>    It  hardly depends on the .bit size and the flash size
> 
>> 3. prompt -> the same situation as is checkboard function
> 
>    it is nice to see what version are you using. At least it makes me
> a little more sane
> 
>> 4. flash size - cfi take cares about - not important
> 
>    ok, I thought it was important because what i see in other boards
> 
>> 5. max flash sect - just choose one max value.
> 
>    ok
> 
>> 6. mtd relate things -> just generic name should be used -> physmap-flash.0: is
>> good with relation with Linux kernel and physmap driver. It helps you with
>> synchronization through command line.
> 
>   Same as 3, but I can change it, it is less critical
> 
>> 7. Size of ram - xparameters.h
> 
>    I add this because a requeriment by Stefan
> 
>> 8. Stefan please correct me but I think that u-boot doesn't handle sector size
>> for storing variables. If no - This should be a small problem that boards have
>> different sect size -> this cause error with sector boundary. -> this should be
>> written in readme file.
>>
>> And that's all. I haven't seen nothing why is this style oriented to board.
> 
>    We have a generic board and specific boards that can overwrite the
> generic functions and add more functionality like custom link script,
> custom xparameters and custom boot, My opinion is that it is style
> oriented
> 
> 
>> On the base what I see in your patch is. That you create board folder with one
>> important file which is xparameters.h for every board. This file contains max 10
>> important value which are hw desing specific and they are no relation with
>> board. They have only relation with hw design which you build and which you can
>> change as you want. There is nothing what solve real problem. IMHO this is too
>> little for adding any specific as new xilinx board is.
> 
> A simple board will just replace the xparameters.h file in
> ppc440-generic folder, more complex boards stil need a folder with
> their initialitation rutines. Also, as a "comertial matter" I believe
> that it is a good idea that the most/sold boards have their folder.
> Future collaborations will have a place to put their work and will be
> more visual to common users.
> 
>> That's exactly what I wrote.
>> Use proper BSP not for generation linux parameters but u-boot specific and take
>> care about name. This solves a lot of problems which can come.
>>
> 
> I dont like this approach, more pain for the user, who will only
> obtain "beautiful" names in the defines. The bsp also has to be very
> updated.  Please no more BSPs. If in the future this BSP is included
> in EDK I will change all the defines.
> 
>> I understand that you are scared about using special bsp for u-boot and for
>> linux. But adding support to u-boot bsp should be easy and a create some month
>> ago special bsp called top which just create a wrapper on every useful bps and
>> run them.
>> In my case I only set top bsp which pass information to u-boot bsp, petalinux,
>> ecos, fdt and some others which I use for developing. Adding one more for older
>> ppc boards which can't handle fdt is easy.
>>
>> I hope you understand me.
> 
> I understand you, but I believe that un fdt and one xparameters.h file
> is more than enough... More xparameters.h with the same information
> repeated again and again is a pain.
> 
> 
> 
>      Thanks again for your comments
> 


More information about the U-Boot mailing list