[U-Boot-Users] [PATCH] for Xilinx's ml300 board

[U-Boot-Users] [PATCH] for Xilinx's ml300 board

[Sean Chang]

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 

[U-Boot-Users] [PATCH] for Xilinx's ml300 board

[Andrew May]

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

[U-Boot-Users] [PATCH] for Xilinx's ml300 board

[sean chang]

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 at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users

[U-Boot-Users] [PATCH] for Xilinx's ml300 board

[Peter Ryser]

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

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

[U-Boot-Users] [PATCH] for Xilinx's ml300 board

[Andrew May]

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.

[U-Boot-Users] [PATCH] for Xilinx's ml300 board

[Wolfgang Denk]

Dear Sean,

in message <Pine.GSO.4.44.0406290904240.13492-101000@blast5> you wrote:
> 
> 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.
...

And later:

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

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!

[U-Boot-Users] [PATCH] for Xilinx's ml300 board

[Andrew May]

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

[U-Boot-Users] [PATCH] for Xilinx's ml300 board

[Sean Chang]

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


Andrew May wrote:
> 
> 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.
> 
>   ------------------------------------------------------------------------
> 
>    iic_adapter.cName: iic_adapter.c
>                 Type: application/x-unknown-content-type-c_auto_file