for Xilinx's ml300 board

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.

Any comments are welcomed.

Thanks,
Sean
==================================

CHANGELOG:
Patch by Sean Chang, 28 Jun 2004:
Add support for ml300 board to read out its environment
information stored on the EEPROM.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ml300.patch.gz
Type: application/octet-stream
Size: 20782 bytes
Desc:
Url : http://lists.denx.de/pipermail/u-boot/attachments/20040629/ff9348fb/attachment.obj

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

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

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.

Another solution might be that we define were CROM is located in the address space. It could be in the DCR address space to not fragment the PLB address space. However, since even the DCR address space can be defined by the user it might be difficult to reserve a location for CROM.

A problem is the association with the processor in a multi-processor system. Let's assume you have a chip with two PPCs on the same PLB and one uses some peripherals were the other uses the other peripherals. xparameters.h resolves the association at compile time whereas CROM would need to contain information about which peripherals are used by which processor.

I'm interested in discussing suggestions on how to solve this problem so that SW application do not need to be recompiled and can gather HW information from CROM (or maybe something else) at run time.

- Peter

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.

Dear Sean,

...

And later:

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?

Best regards,

Wolfgang Denk

Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
Boykottiert Microsoft - Kauft Eure Fenster bei OBI!

...

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: iic_adapter.c
Type: text/x-csrc
Size: 14461 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20040713/0f1f985c/attachment.c

Hi,

Thanks Andrew. I have the patch ready but am currently waiting for internal approval. Will submit it as soon as it's okayed. Feel free to let me know if you have further comment/suggestion.

Thanks,
Sean