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

[Ricardo Ribalda Delgado]

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

-- 
Ricardo Ribalda
http://www.eps.uam.es/~rribalda/