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

[Michal Simek]

> Hi Stefan

You missed my name but ok.

>   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?

read Menon email

>> 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.

yes it is easy - just copy xparameters.h
> 
> I can add something like
> #ifdef XPAR_OTHER_RAM_NAME
>   #define #XPAR_DDR2_SDRAM_MEM_BASEADDR XPAR_OTHER_RAM_NAME
> #endif

no you needn't - just you bsp - bsp take care about.

>> 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

Yes I agree with that we should keep one representative board with use generic
ppc platform but just one not more. I vote for xilinx ml507. It is enought.

>> 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

If user using generic design he has set standalone bsp in edk - no names. If he
wants to play with linux or u-boot just fill the name which he wants to see in
prompt. Bsp add U-BOOT to begin of command line. (for example U-BOOT-ML507>).
This is no problem.

> 
>> 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.
ok.

> 
>> 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.

I understand. If you want I can check it on Microblaze too. But be aware that
this driver should not be based on xilinx files.

> 
>> 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

Size of .bit is not a problem because you can alloc 2MB that should be enough
for every bitstream. For env size choose one sector. This is about layout of
flash (MTD). User can choose and I believe will choose his own layout which is
the best for him.

> 
>> 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

User use a lot of design -> he use a lot project. He can choose different name
for his project but again this is better to setup in EDK where he 100% will do a
design.

> 
>> 4. flash size - cfi take cares about - not important
> 
>    ok, I thought it was important because what i see in other boards

I don't remember but this is not important you can add it to xparameters too.

>> 5. max flash sect - just choose one max value.
>
>    ok

[snip]

>> 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

[snip]

>> 7. Size of ram - xparameters.h
> 
>    I add this because a requeriment by Stefan

yes. Size is important and you can use more generic way.

>> 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.

Yes. I understand reason why should user have create his own folder with his
design. It is important but again this is really user specific things. If he
want to see on every startup "Hello you are the best, my hero", he can change
what he wants but this is not for mainline u-boot.

> 
> 
>> 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.

What is your simple board? Every board is simple if you know it. But you meant
simple design. That a big difference.

Yes. commercial matter is important. We can add thousand of design in microblaze
or ppc405,ppc440 folders and Wolfgang could write that u-boot supports thousand
boards. But the real state will be different. And the same from Xilinx and Avnet
site but what will be better to support thousands board (or design) or just full
ppc440 based on xilinx fpga. What is bigger? And who choose what boards are the
most/sold. I don't have numbers.
The next things which can comes in after 2 years. You are board maintainer for
ml507 and this board will be ancient but U-BOOT will contain this board and will
we move several years till someone just remove it. The same situation comes with
others board.
In generic style - generic platform will be in U-BOOT forever and one example
board no one cares.

>> 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 haven't invented BSP style in EDK. You have to choose linux one if you want to
work with linux. I believe you are not lazy to choose different one.
For example fdt bsp which I started with it (currently is maintained by Xilinx)
should be in EDK or maybe is. EDK is not standard and the things there are not
fully up to date. I will talked with people from Xilinx if is possible to and
which are their requirements for add u-boot bsp to EDK.

This is different theme and I don't want to disturb people in u-boot mailing
list. This is only about name. It is up to Stefan which names are acceptable for
him.


>> 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.

You are wrong. Xparameters.h is looooooong file where 98% of values are
unneeded. In Loong file with lot of information you lose everything. Maybe you
will be happy that you have looooong file and there is a lot of information but
the purpose of this file is...
Loong xparameters.h was unacceptable for linux-kernel in ppc branch and is in
EDK due to compatibility. FDT solved this.


>      Thanks again for your comments
> 

OK. I think we found a solution.

I agree that your generic patch is better than adding next platform.
If you can include changes which I report in previous email and resend, it will
be great.
Add only ml507 and small xparameters.h with values which are used not more.

Stefan: you are ppc440 custodian. I would like to see some comments from you.

Michal