Hi,

Sorry for the mess. Apparently I missed a step in cleaning up the spacing for my patch. Please ignore this patch and I'll submit another one soon.

Andrew, your points are well taken. I was planning on doing this in different steps but it doesn't seem like a good idea after all. I'll separate out the non-standard environment functions from IIC support and re-submit the patch.

Further comments/suggestions are welcomed.

Thanks, Sean

Andrew May wrote:

On Tue, Jun 29, 2004 at 09:06:51AM -0700, Sean Chang wrote:

...

Hi Wolfgang,

This is the first time I am submitting a patch, so if there is any problem please let me know and I'll be more than happy to fix it.

This patch is applied against the u-boot 1.1.1 release. Basically it adds support for the ml300 board to read out its environment information stored on the EEPROM.

I would think the more important feature is that IIC support is added. I also wouldn't skip the fact that the OS independent code has changes as well. There are also a lot of tab to spaces conversions, that hide the real changes.

It would be up to Wolfgang if he wants the stuff redone, but I would like to see a more complete change log. like

CHANGELOG: Patch by Sean Chang, 28 Jun 2004: Updates to Virtex-II Pro/ml300 board OS independent code to Xilinx EDK 6.2 Added IIC support (with non-standard environment functions) Add support for ml300 board to read out its environment information stored on the EEPROM. Convert Xilinx style environment vars to U-boot style vars.

The non-standard env stuff should really be moved to a separate file so people can use just the IIC support without pulling that garbage in.

The whole xparameters.h file is really annoying. It makes the IPIF code impossible to use on more than one board type without a recompile. That may be OK for U-boot but it is horrible in the Linux kernel.

These things really need some ()'s. +#define XIIC_CR_REG_OFFSET 0x00+XIIC_REG_OFFSET /* Control Register */

---

This SF.Net email sponsored by Black Hat Briefings & Training. Attend Black Hat Briefings & Training, Las Vegas July 24-29 - digital self defense, top technical experts, no vendor pitches, unmatched networking opportunities. Visit www.blackhat.com _______________________________________________ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users

On Wed, 2004-06-30 at 07:30, Peter Ryser wrote:

...

The whole xparameters.h file is really annoying. It makes the IPIF code impossible to use on more than one board type without a recompile. That may be OK for U-boot but it is horrible in the Linux kernel.

Unfortunately, we have not yet found a good way to store the hardware information (let's call it CROM) as part of the bitstream in a way that it becomes easily available to the software. The hardware system is very flexible, i.e. the location where such information is stored may be different for various boards (bitstreams).

One solution might be to pass the location of the CROM as part of the boot parameters to the Linux kernel or even go one step further and pass the whole HW configuration as a boot parameter to the Linux kernel. A similar technique might be doable for u-boot.

My problem isn't really that I want to get the entire config from hardware, but with the way the xparamters.h file is compiled into static arrays. So the whole point of the doing the get the device by the ID is wasted.

Here is a walk through of the current code.

In xparameters.h the defines. =================================== #define XPAR_XEMAC_NUM_INSTANCES 1 #define XPAR_OPB_ETHERNET_0_BASEADDR 0x60000000 #define XPAR_OPB_ETHERNET_0_HIGHADDR 0x60003FFF #define XPAR_OPB_ETHERNET_0_DEVICE_ID 0 =================================== Then the eth_init() which calls this init Result = XEmac_Initialize(&Emac, XPAR_EMAC_0_DEVICE_ID); =================================== which calls this lookup XEmac_LookupConfig(DeviceId); =================================== and then the lookup uses the table XEmac_Config * XEmac_LookupConfig(u16 DeviceId) { XEmac_Config *CfgPtr = NULL; int i;

for (i = 0; i < XPAR_XEMAC_NUM_INSTANCES; i++) { if (XEmac_ConfigTable[i].DeviceId == DeviceId) { CfgPtr = &XEmac_ConfigTable[i]; break; } }

return CfgPtr; } =================================== and then in a seperate file the actual table is statically defined with same values from the xparmeters.h

XEmac_Config XEmac_ConfigTable[] = { { XPAR_OPB_ETHERNET_0_DEVICE_ID, XPAR_OPB_ETHERNET_0_BASEADDR, XPAR_OPB_ETHERNET_0_ERR_COUNT_EXIST, XPAR_OPB_ETHERNET_0_DMA_PRESENT, XPAR_OPB_ETHERNET_0_MII_EXIST} }; =================================== Now for u-boot you could really argue the whole chain of calls is a complete waste of space. You are just using one define to get a set of them that are compiled into the code, when it would just be as easy to use defines directly in the XEmac_Initialize() call without going through the middle man/functions.

Again for u-boot this is probably overkill, but for Linux it would really help to be able to use one image on more than one board easily.

A simple change that would go a long way to help is to have the XEmac_ConfigTable be set at run time. something like XEmac_Config *XEmac_ConfigTable;

XEmac_LookupConfig(u16 DeviceId) { if( XEmac_ConfigTable == NULL ){ int bt; bt = get_board_type();/*Checks simple HW reg or env var*/ if( bt == 2 ){ XEmac_ConfigTable = XEmac_table_2mac; }else{ XEmac_ConfigTable = XEmac_table_1mac; } } /*Fixup table to be null terminated or move the size into *a wrapping struct instead of the constant. */ ..... } Since I don't think it is really a u-boot issue we can try to take this off to another list.

On Sun, Jul 11, 2004 at 06:26:24PM +0200, Wolfgang Denk wrote:

Dear Sean,

...

Just to make sure there was no misunderstanding on my side: I did ignore your first patch, but I haven't seen a newer one yet. My understanding is that you are still working on this. Is this correct, or did I miss something?

I did just get around to playing with the patch to get the generic i2c stuff working, but I did not cleanup the other stuff.

My changes were to iic_adapter.c to pull out the Xilinix EEPROM assumptions. I was able to do a quick read of a LM83 chip from the imd commands.

If we don't hear anything from Sean soon, I will try to do the patch.

Here is the iic_adapter.c file I have, in case Sean is already working on changing it.