[U-Boot-Users] [PATCH] m68k fix for Ethernet Buffers at address 0
Hi,
Attached is a patch that fixes a problem i saw with a coldfire 5235 port. i'll include the patch here in the email as well since it describes the problem:
diff -purN u-boot-1.1.6/lib_m68k/board.c u-boot-1.1.6-pktfix/lib_m68k/ board.c --- u-boot-1.1.6/lib_m68k/board.c 2006-11-02 09:15:01.000000000 -0500 +++ u-boot-1.1.6-pktfix/lib_m68k/board.c 2007-06-28 11:07:19.000000000 -0400 @@ -642,6 +642,22 @@ void board_init_r (gd_t *id, ulong dest_ #if (CONFIG_COMMANDS & CFG_CMD_NET) && defined(FEC_ENET) WATCHDOG_RESET(); + + /* init NetRxPackets here otherwise eth_init() will cause + * buffer descriptors to use 0 as their buffer and overwrite lower memory. + * NetLoop() does this properly later. + * Cannot remove eth_init() call because linux driver assumes this + * happened, for MAC Address to be loaded into microcontroller + */ + { + int i; + static uchar pkt[(PKTBUFSRX+1) * PKTSIZE_ALIGN + PKTALIGN]; + uchar *tx = pkt + (PKTALIGN - 1); + tx -= (ulong) tx % PKTALIGN; + for (i = 0; i < PKTBUFSRX; i++) { + NetRxPackets[i] = tx + (i+1)*PKTSIZE_ALIGN; + } + } eth_init(bd); #endif
Dear Wilson,
in message 04AE3AAD-E24E-4EAE-AA2B-5B7CFA0CA026@savantav.com you wrote:
Attached is a patch that fixes a problem i saw with a coldfire 5235 port. i'll include the patch here in the email as well since it describes the problem:
First, a formal issue: you missed to include the Signed-off-by: line which is mandatory for each patch.
Second, a technical issue:
* Cannot remove eth_init() call because linux driver assumes this* happened, for MAC Address to be loaded into microcontroller
This is a problem with the Linux driver then, and should be fixed in Linux. Please see the FAQ entry about this, and then let's get rid of this eth_init call in U-Boot. Philosophy here is that devices shall be only initialized when really used by U-Boot.
Best regards,
Wolfgang Denk
Dear Wolfgang,
First, a formal issue: you missed to include the Signed-off-by: line which is mandatory for each patch.
Apologies. I didnt know i should "sign off" my own patches, but now i see that is what i'm supposed to do.
This is a problem with the Linux driver then, and should be fixed in Linux. Please see the FAQ entry about this, and then let's get rid of this eth_init call in U-Boot. Philosophy here is that devices shall be only initialized when really used by U-Boot.
that makes sense. in this case its u-boot that knows the Ethernet Address since its stored in its environment. Possibly the solution is to create a routine in fec.c (the fast ethernet driver) which only sets the MAC in the coldfire, instead of starting up the ethernet port before the buffers are ready. This change would keep things working as they are, but fix the bug where packets are received before the buffer address is set.
should i go ahead with this fix, or instead remove the eth_init() call and risk breaking all the m68k systems?
thanks, wilson
Dear Wilson,
in message 97339EDB-05CD-4EFA-BF76-28D15E6D7633@savantav.com you wrote:
This is a problem with the Linux driver then, and should be fixed in Linux. Please see the FAQ entry about this, and then let's get rid of this eth_init call in U-Boot. Philosophy here is that devices shall be only initialized when really used by U-Boot.
that makes sense. in this case its u-boot that knows the Ethernet Address since its stored in its environment. Possibly the solution is to create a routine in fec.c (the fast ethernet driver) which only
If you mean the U-Boot file: then no, that would be the wrong approach.
U-Boot shall initialize a device only when it uses it. If Ethernet is not used by U-Boot itself, it shall not touch any parts of the Ethernet controller.
U-Boot shall pass the MAC address to the Linux kernel by whatever means the current architecture provides; if no other means are available for example on the kernel command line.
sets the MAC in the coldfire, instead of starting up the ethernet port before the buffers are ready. This change would keep things working as they are, but fix the bug where packets are received before the buffer address is set.
Which bug are you talking about? Is this in U-Boot or in Linux?
Best regards,
Wolfgang Denk
On 7/13/07, Wolfgang Denk wd@denx.de wrote:
U-Boot shall initialize a device only when it uses it. If Ethernet is not used by U-Boot itself, it shall not touch any parts of the Ethernet controller.
Ok, now I'm confused.
When I submitted the macb ethernet driver, which works on both avr32 and at91sam926x devices, for inclusion in the LInux kernel, I was told to remove the mac address from the platform data. This was because ARM relied on u-boot initializing the mac address of all ethernet controllers before booting Linux, so AVR32 should do the same as well.
So I did this and added some code to the avr32 board setup to initialize the mac address registers as a temporary measure until I could be sure that u-boot did in fact always set up the mac address on all interfaces.
Now, it seems like the situation is: a) Other architectures rely on mac registers being initialized by u-boot as well. b) This is not how things are supposed to work.
I'd be happy to go back to passing mac addresses through the tagged list (in fact, u-boot never stopped doing that on avr32) but this needs to be resolved on ARM before I can change the driver.
Håvard
In message 1defaf580707121538y65db81c9y748f14b9a62ea31b@mail.gmail.com you wrote:
When I submitted the macb ethernet driver, which works on both avr32 and at91sam926x devices, for inclusion in the LInux kernel, I was told to remove the mac address from the platform data. This was because ARM
So far this is most probably correct.
relied on u-boot initializing the mac address of all ethernet controllers before booting Linux, so AVR32 should do the same as well.
This is definitely not correct. U-Boot's position about this is very clear, and has been for a long time. This is actually a FAQ, please look it up.
So I did this and added some code to the avr32 board setup to initialize the mac address registers as a temporary measure until I could be sure that u-boot did in fact always set up the mac address on all interfaces.
It will never do that.
Now, it seems like the situation is: a) Other architectures rely on mac registers being initialized by u-boot as well. b) This is not how things are supposed to work.
b)
I'd be happy to go back to passing mac addresses through the tagged list (in fact, u-boot never stopped doing that on avr32) but this needs to be resolved on ARM before I can change the driver.
The thing is that the ARM folks don't want o see MAC addresses in ATAGs either. Don't ask me why - if they use this mechanism for passing h/w related information around that would seem the most logical approach to me, too. But they don't want it.
And we are as stubborn as them: we don't want to initialize h/w we didn't use.
<asbestos> The Right Thing (TM) would of course be to get rid of all this ATAGs stuff on ARM and use a proper device tree there, too, like the PowerPC folks have been doing for such a long time. And on MIPS and all the other systems, too. </asbestos>
[He, I'm leaving for vacation, so I won't suffer from the fires of the flame war I just started :-) ]
Best regards,
Wolfgang Denk
On 7/14/07, Wolfgang Denk wd@denx.de wrote:
In message 1defaf580707121538y65db81c9y748f14b9a62ea31b@mail.gmail.com you wrote:
When I submitted the macb ethernet driver, which works on both avr32 and at91sam926x devices, for inclusion in the LInux kernel, I was told to remove the mac address from the platform data. This was because ARM
So far this is most probably correct.
relied on u-boot initializing the mac address of all ethernet controllers before booting Linux, so AVR32 should do the same as well.
This is definitely not correct. U-Boot's position about this is very clear, and has been for a long time. This is actually a FAQ, please look it up.
Well, the mac address must be passed _somewhere_, so they are either both correct or both incorrect. But after reading the FAQ and the linux-arm thread it links to, it's clear to me that AVR32 is right and ARM is wrong, i.e. passing the mac address through the boot protocol (ATAGs or otherwise) is the right thing to do.
So I will just forget about "fixing" u-boot and instead consider posting a patch for the macb driver to look for the mac address in the platform_data instead of the hardware registers. I don't have the time to start a flamewar right now, so it'll have to wait until after my vacation at least.
I'd be happy to go back to passing mac addresses through the tagged list (in fact, u-boot never stopped doing that on avr32) but this needs to be resolved on ARM before I can change the driver.
The thing is that the ARM folks don't want o see MAC addresses in ATAGs either. Don't ask me why - if they use this mechanism for passing h/w related information around that would seem the most logical approach to me, too. But they don't want it.
Yeah, I got the impression that they wanted to break nfsroot just for the heck of it...
<asbestos> The Right Thing (TM) would of course be to get rid of all this ATAGs stuff on ARM and use a proper device tree there, too, like the PowerPC folks have been doing for such a long time. And on MIPS and all the other systems, too. </asbestos>
Heh. AVR32 uses ATAGs too, so I suppose you want me to switch as well?
I guess I should look into it. Switching boot protocol sounds like a long and painful experience though...
[He, I'm leaving for vacation, so I won't suffer from the fires of the flame war I just started :-) ]
People don't seem to be taking the bait so far ;-)
Håvard
Hi,
back in u-boot-1.1.4 there was an option CONFIG_BOOTP_SERVER which controlled if the bootp server was the tftp server. in u-boot-1.1.6 it has been removed, but i find that option quite useful because you can program the tftp server using the environment variable serverip.
I have restored the option and included it in CONFIG_BOOTP_DEFAULT so the result is the same operation to all boards that dont remove it from their CONFIG_BOOTP_MASK.
CHANGELOG:
restored CONFIG_BOOTP_SERVER from previous u-boot code to control the tftp server vs the bootp server
Signed-off-by: Wilson Callan wcallan@savantav.com
Hi Wilson,
back in u-boot-1.1.4 there was an option CONFIG_BOOTP_SERVER which controlled if the bootp server was the tftp server. in u-boot-1.1.6 it has been removed, but i find that option quite useful because you can program the tftp server using the environment variable serverip.
Can you help me to find that in the original sources? I seem not to be able to do that:
[dzu@pollux u-boot]$ git checkout U-Boot-1_1_4 [dzu@pollux u-boot]$ head -2 CHANGELOG | tail -1 Changes for U-Boot 1.1.4: [dzu@pollux u-boot]$ grep -ri CONFIG_BOOTP_SERVER net/ [dzu@pollux u-boot]$
Flexing my git-muscle, I cannot find a reference to it anywhere in the history of the networking code:
[dzu@pollux u-boot]$ git checkout master Switched to branch "master" [dzu@pollux u-boot]$ git log -p net/ | grep BOOTP_SERVER [dzu@pollux u-boot]$
What exact version did you see that option in?
Cheers Detlev
ah, you are correct. it looks like i added it back in my 1.1.4 port and missed that fact when porting to 1.1.6. So this is a submit for a new option then. I hope others find it useful. It fits in well with the existing serverip environment variable.
CHANGELOG:
Added CONFIG_BOOTP_SERVER to allow the tftp server to be different from the bootp server
Signed-off-by: Wilson Callan wcallan@savantav.com
On Jul 23, 2007, at 5:07 AM, Detlev Zundel wrote:
Hi Wilson,
back in u-boot-1.1.4 there was an option CONFIG_BOOTP_SERVER which controlled if the bootp server was the tftp server. in u-boot-1.1.6 it has been removed, but i find that option quite useful because you can program the tftp server using the environment variable serverip.
Can you help me to find that in the original sources? I seem not to be able to do that:
[dzu@pollux u-boot]$ git checkout U-Boot-1_1_4 [dzu@pollux u-boot]$ head -2 CHANGELOG | tail -1 Changes for U-Boot 1.1.4: [dzu@pollux u-boot]$ grep -ri CONFIG_BOOTP_SERVER net/ [dzu@pollux u-boot]$
Flexing my git-muscle, I cannot find a reference to it anywhere in the history of the networking code:
[dzu@pollux u-boot]$ git checkout master Switched to branch "master" [dzu@pollux u-boot]$ git log -p net/ | grep BOOTP_SERVER [dzu@pollux u-boot]$
What exact version did you see that option in?
Cheers Detlev
-- Man is a fool, and woman, for tolerating him, is a damned fool -- Mark Twain -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.de
Hi Wilson,
ah, you are correct. it looks like i added it back in my 1.1.4 port and missed that fact when porting to 1.1.6. So this is a submit for a new option then. I hope others find it useful. It fits in well with the existing serverip environment variable.
I agree - lets wait until Ben picks it up.
Cheers Detlev
Hi Wilson,
On Mon, 2007-07-23 at 18:36 +0200, Detlev Zundel wrote:
Hi Wilson,
ah, you are correct. it looks like i added it back in my 1.1.4 port and missed that fact when porting to 1.1.6. So this is a submit for a new option then. I hope others find it useful. It fits in well with the existing serverip environment variable.
I agree - lets wait until Ben picks it up.
Cheers Detlev
This looks good. Sorry for being so picky, but please do the following and I'll integrate right away:
1. Since we're not restoring anything, the e-mail title needs to change 2. CHANGELOG part is no longer needed 3. Change e-mail body to a short description of the patch
If possible, please paste the patch into the message instead of attaching. Not a big deal, but it's a little easier for me that way.
thanks, Ben
Added CONFIG_BOOTP_SERVER to allow the tftp server to be different from the bootp server
Signed-off-by: Wilson Callan wcallan@savantav.com
--- u-boot-1.1.6/include/cmd_confdefs.h 2006-11-02 09:15:01.000000000 -0500 +++ u-boot/include/cmd_confdefs.h 2007-05-23 15:02:04.000000000 -0400 @@ -167,6 +167,7 @@ #define CONFIG_BOOTP_SEND_HOSTNAME 0x00000100 #define CONFIG_BOOTP_NTPSERVER 0x00000200 #define CONFIG_BOOTP_TIMEOFFSET 0x00000400 +#define CONFIG_BOOTP_SERVER 0x00000800 #define CONFIG_BOOTP_VENDOREX 0x80000000 @@ -176,7 +177,8 @@ #define CONFIG_BOOTP_DEFAULT (CONFIG_BOOTP_SUBNETMASK | \ CONFIG_BOOTP_GATEWAY | \ CONFIG_BOOTP_HOSTNAME | \ - CONFIG_BOOTP_BOOTPATH) + CONFIG_BOOTP_BOOTPATH | \ + CONFIG_BOOTP_SERVER) #ifndef CONFIG_BOOTP_MASK #define CONFIG_BOOTP_MASK CONFIG_BOOTP_DEFAULT --- u-boot-1.1.6/net/bootp.c 2006-11-02 09:15:01.000000000 -0500 +++ u-boot/net/bootp.c 2007-05-21 11:43:15.000000000 -0400 @@ -120,10 +120,12 @@ IPaddr_t tmp_ip; NetCopyIP(&NetOurIP, &bp->bp_yiaddr); +#if (CONFIG_BOOTP_MASK & CONFIG_BOOTP_SERVER) NetCopyIP(&tmp_ip, &bp->bp_siaddr); if (tmp_ip != 0) NetCopyIP(&NetServerIP, &bp->bp_siaddr); memcpy (NetServerEther, ((Ethernet_t *)NetRxPkt)->et_src, 6); +#endif if (strlen(bp->bp_file) > 0) copy_filename (BootFile, bp->bp_file, sizeof(BootFile));
On Jul 25, 2007, at 11:51 AM, Ben Warren wrote:
- Since we're not restoring anything, the e-mail title needs to
change 2. CHANGELOG part is no longer needed 3. Change e-mail body to a short description of the patch
If possible, please paste the patch into the message instead of attaching.
Wilson,
On Thu, 2007-07-26 at 12:08 -0400, Wilson Callan wrote: <snip>
On Jul 25, 2007, at 11:51 AM, Ben Warren wrote:
- Since we're not restoring anything, the e-mail title needs to
change 2. CHANGELOG part is no longer needed 3. Change e-mail body to a short description of the patch
If possible, please paste the patch into the message instead of attaching.
Almost there. I get a 'malformed patch' error when I try to apply this. I don't think it's whitespace-related, but whatever you're using for diff doesn't play well with git.
Please re-submit, and make sure the patch is against the latest git snapshot, not v1.1.6:
'git clone git://www.denx.de/git/u-boot.git'
The best way to ensure that the patch will apply is to use 'git diff' instead of 'diff -...'
thanks, Ben
Added CONFIG_BOOTP_SERVER to allow the tftp server to be different from the bootp server
Signed-off-by: Wilson Callan wcallan@savantav.com
diff --git a/include/cmd_confdefs.h b/include/cmd_confdefs.h index b3ccdce..581dbf7 100644 --- a/include/cmd_confdefs.h +++ b/include/cmd_confdefs.h @@ -169,6 +169,7 @@ #define CONFIG_BOOTP_SEND_HOSTNAME 0x00000100 #define CONFIG_BOOTP_NTPSERVER 0x00000200 #define CONFIG_BOOTP_TIMEOFFSET 0x00000400 +#define CONFIG_BOOTP_SERVER 0x00000800 #define CONFIG_BOOTP_VENDOREX 0x80000000 @@ -178,7 +179,8 @@ #define CONFIG_BOOTP_DEFAULT (CONFIG_BOOTP_SUBNETMASK | \ CONFIG_BOOTP_GATEWAY | \ CONFIG_BOOTP_HOSTNAME | \ - CONFIG_BOOTP_BOOTPATH) + CONFIG_BOOTP_BOOTPATH | \ + CONFIG_BOOTP_SERVER) #ifndef CONFIG_BOOTP_MASK #define CONFIG_BOOTP_MASK CONFIG_BOOTP_DEFAULT diff --git a/net/bootp.c b/net/bootp.c index 1de9a8f..2c05f2e 100644 --- a/net/bootp.c +++ b/net/bootp.c @@ -120,10 +120,12 @@ static void BootpCopyNetParams(Bootp_t *bp) IPaddr_t tmp_ip; NetCopyIP(&NetOurIP, &bp->bp_yiaddr); +#if (CONFIG_BOOTP_MASK & CONFIG_BOOTP_SERVER) NetCopyIP(&tmp_ip, &bp->bp_siaddr); if (tmp_ip != 0) NetCopyIP(&NetServerIP, &bp->bp_siaddr); memcpy (NetServerEther, ((Ethernet_t *)NetRxPkt)->et_src, 6); +#endif if (strlen(bp->bp_file) > 0) copy_filename (BootFile, bp->bp_file, sizeof(BootFile));
On Fri, 2007-07-27 at 09:25, Wilson Callan wrote:
Added CONFIG_BOOTP_SERVER to allow the tftp server to be different from the bootp server
Signed-off-by: Wilson Callan wcallan@savantav.com
diff --git a/include/cmd_confdefs.h b/include/cmd_confdefs.h index b3ccdce..581dbf7 100644 --- a/include/cmd_confdefs.h +++ b/include/cmd_confdefs.h @@ -169,6 +169,7 @@ #define CONFIG_BOOTP_SEND_HOSTNAME 0x00000100 #define CONFIG_BOOTP_NTPSERVER 0x00000200 #define CONFIG_BOOTP_TIMEOFFSET 0x00000400 +#define CONFIG_BOOTP_SERVER 0x00000800 #define CONFIG_BOOTP_VENDOREX 0x80000000 @@ -178,7 +179,8 @@ #define CONFIG_BOOTP_DEFAULT (CONFIG_BOOTP_SUBNETMASK | \ CONFIG_BOOTP_GATEWAY | \ CONFIG_BOOTP_HOSTNAME | \
CONFIG_BOOTP_BOOTPATH)
CONFIG_BOOTP_BOOTPATH | \CONFIG_BOOTP_SERVER)#ifndef CONFIG_BOOTP_MASK #define CONFIG_BOOTP_MASK CONFIG_BOOTP_DEFAULT diff --git a/net/bootp.c b/net/bootp.c index 1de9a8f..2c05f2e 100644 --- a/net/bootp.c +++ b/net/bootp.c @@ -120,10 +120,12 @@ static void BootpCopyNetParams(Bootp_t *bp) IPaddr_t tmp_ip; NetCopyIP(&NetOurIP, &bp->bp_yiaddr); +#if (CONFIG_BOOTP_MASK & CONFIG_BOOTP_SERVER) NetCopyIP(&tmp_ip, &bp->bp_siaddr); if (tmp_ip != 0) NetCopyIP(&NetServerIP, &bp->bp_siaddr); memcpy (NetServerEther, ((Ethernet_t *)NetRxPkt)->et_src, 6); +#endif if (strlen(bp->bp_file) > 0) copy_filename (BootFile, bp->bp_file, sizeof(BootFile));
Any chance we can get this patch rebased on the version of things in the u-boot-testing tree? It would save everyone a bunch of trouble!
Thanks, jdl
Added CONFIG_BOOTP_SERVERIP to allow the tftp server to be different from the bootp server
Signed-off-by: Wilson Callan wcallan@savantav.com
diff --git a/README b/README index cfbd39a..d7812a3 100644 --- a/README +++ b/README @@ -1141,6 +1141,9 @@ The following options need to be configured: CONFIG_BOOTP_TIMEOFFSET CONFIG_BOOTP_VENDOREX
+ CONFIG_BOOTP_SERVERIP - TFTP server will be the serverip + environment variable, not the BOOTP server. + CONFIG_BOOTP_DNS2 - If a DHCP client requests the DNS serverip from a DHCP server, it is possible that more than one DNS serverip is offered to the client. @@ -1153,7 +1156,7 @@ The following options need to be configured: CONFIG_BOOTP_SEND_HOSTNAME - Some DHCP servers are capable to do a dynamic update of a DNS server. To do this, they need the hostname of the DHCP requester. - If CONFIG_BOOP_SEND_HOSTNAME is defined, the content + If CONFIG_BOOTP_SEND_HOSTNAME is defined, the content of the "hostname" environment variable is passed as option 12 to the DHCP server.
diff --git a/net/bootp.c b/net/bootp.c index 80f53bc..be1ee33 100644 --- a/net/bootp.c +++ b/net/bootp.c @@ -120,10 +120,12 @@ static void BootpCopyNetParams(Bootp_t *bp) IPaddr_t tmp_ip;
NetCopyIP(&NetOurIP, &bp->bp_yiaddr); +#if !defined(CONFIG_BOOTP_SERVERIP) NetCopyIP(&tmp_ip, &bp->bp_siaddr); if (tmp_ip != 0) NetCopyIP(&NetServerIP, &bp->bp_siaddr); memcpy (NetServerEther, ((Ethernet_t *)NetRxPkt)->et_src, 6); +#endif if (strlen(bp->bp_file) > 0) copy_filename (BootFile, bp->bp_file, sizeof(BootFile));
On Jul 27, 2007, at 11:54 AM, Jon Loeliger wrote:
Any chance we can get this patch rebased on the version of things in the u-boot-testing tree? It would save everyone a bunch of trouble!
Thanks, jdl
In message 4D62CEA2-9134-4970-9570-5605F91A94A7@savantav.com you wrote:
Added CONFIG_BOOTP_SERVERIP to allow the tftp server to be different from the bootp server
Can you please explain why this is needed?
Best regards,
Wolfgang Denk
In message FE6E7FFD-AF24-456E-A9D3-56A616093032@savantav.com you wrote:
Added CONFIG_BOOTP_SERVER to allow the tftp server to be different from the bootp server
Sorry for the late review...
Which problem is this patch supposed to fix? Doesn't the "nextserver" option in your DHCP server config file do exactly what you want?
index b3ccdce..581dbf7 100644 --- a/include/cmd_confdefs.h +++ b/include/cmd_confdefs.h @@ -169,6 +169,7 @@ #define CONFIG_BOOTP_SEND_HOSTNAME 0x00000100 #define CONFIG_BOOTP_NTPSERVER 0x00000200 #define CONFIG_BOOTP_TIMEOFFSET 0x00000400 +#define CONFIG_BOOTP_SERVER 0x00000800 #define CONFIG_BOOTP_VENDOREX 0x80000000 @@ -178,7 +179,8 @@ #define CONFIG_BOOTP_DEFAULT (CONFIG_BOOTP_SUBNETMASK | \ CONFIG_BOOTP_GATEWAY | \ CONFIG_BOOTP_HOSTNAME | \
CONFIG_BOOTP_BOOTPATH)
CONFIG_BOOTP_BOOTPATH | \CONFIG_BOOTP_SERVER)#ifndef CONFIG_BOOTP_MASK #define CONFIG_BOOTP_MASK CONFIG_BOOTP_DEFAULT diff --git a/net/bootp.c b/net/bootp.c index 1de9a8f..2c05f2e 100644 --- a/net/bootp.c +++ b/net/bootp.c @@ -120,10 +120,12 @@ static void BootpCopyNetParams(Bootp_t *bp) IPaddr_t tmp_ip; NetCopyIP(&NetOurIP, &bp->bp_yiaddr); +#if (CONFIG_BOOTP_MASK & CONFIG_BOOTP_SERVER) NetCopyIP(&tmp_ip, &bp->bp_siaddr); if (tmp_ip != 0) NetCopyIP(&NetServerIP, &bp->bp_siaddr); memcpy (NetServerEther, ((Ethernet_t *)NetRxPkt)->et_src, 6); +#endif
If I understand corrrectly, this patch changes the behaviour for all boards that currently don't define CONFIG_BOOTP_SERVER.
Is this intentional?
Best regards,
Wolfgang Denk
Hi Wolfgang,
In message FE6E7FFD-AF24-456E-A9D3-56A616093032@savantav.com you wrote:
Added CONFIG_BOOTP_SERVER to allow the tftp server to be different from the bootp server
Sorry for the late review...
Which problem is this patch supposed to fix? Doesn't the "nextserver" option in your DHCP server config file do exactly what you want?
I would not say it tries to "fix a problem" but rather "add an option". In my opinion next-server really serves the same purpose, but of course out of reach of U-Boot. So we gain more freedom in regard to misconfigured DHCP servers.
index b3ccdce..581dbf7 100644 --- a/include/cmd_confdefs.h +++ b/include/cmd_confdefs.h @@ -169,6 +169,7 @@ #define CONFIG_BOOTP_SEND_HOSTNAME 0x00000100 #define CONFIG_BOOTP_NTPSERVER 0x00000200 #define CONFIG_BOOTP_TIMEOFFSET 0x00000400 +#define CONFIG_BOOTP_SERVER 0x00000800 #define CONFIG_BOOTP_VENDOREX 0x80000000 @@ -178,7 +179,8 @@ #define CONFIG_BOOTP_DEFAULT (CONFIG_BOOTP_SUBNETMASK | \ CONFIG_BOOTP_GATEWAY | \ CONFIG_BOOTP_HOSTNAME | \
CONFIG_BOOTP_BOOTPATH)
CONFIG_BOOTP_BOOTPATH | \CONFIG_BOOTP_SERVER)#ifndef CONFIG_BOOTP_MASK #define CONFIG_BOOTP_MASK CONFIG_BOOTP_DEFAULT diff --git a/net/bootp.c b/net/bootp.c index 1de9a8f..2c05f2e 100644 --- a/net/bootp.c +++ b/net/bootp.c @@ -120,10 +120,12 @@ static void BootpCopyNetParams(Bootp_t *bp) IPaddr_t tmp_ip; NetCopyIP(&NetOurIP, &bp->bp_yiaddr); +#if (CONFIG_BOOTP_MASK & CONFIG_BOOTP_SERVER) NetCopyIP(&tmp_ip, &bp->bp_siaddr); if (tmp_ip != 0) NetCopyIP(&NetServerIP, &bp->bp_siaddr); memcpy (NetServerEther, ((Ethernet_t *)NetRxPkt)->et_src, 6); +#endif
If I understand corrrectly, this patch changes the behaviour for all boards that currently don't define CONFIG_BOOTP_SERVER.
Is this intentional?
If I understand this correctly, because CONFIG_BOOTP_MASK is tested and I don't see anyone defining this, it will *not* touch any configuration right now. Subsequently for consistency the code in bootp.c _may_ move to also using this MASK variable but then many configs will have to be touched. Many configs potentially can simply move to CONFIG_BOOTP_DEFAULT.
Cheers Detlev
In message m2lkcjvfmw.fsf@ohwell.denx.de you wrote:
Which problem is this patch supposed to fix? Doesn't the "nextserver" option in your DHCP server config file do exactly what you want?
I would not say it tries to "fix a problem" but rather "add an option". In my opinion next-server really serves the same purpose, but of course out of reach of U-Boot. So we gain more freedom in regard to misconfigured DHCP servers.
Hm...
+++ b/include/cmd_confdefs.h @@ -169,6 +169,7 @@ #define CONFIG_BOOTP_SEND_HOSTNAME 0x00000100 #define CONFIG_BOOTP_NTPSERVER 0x00000200 #define CONFIG_BOOTP_TIMEOFFSET 0x00000400 +#define CONFIG_BOOTP_SERVER 0x00000800 #define CONFIG_BOOTP_VENDOREX 0x80000000 @@ -178,7 +179,8 @@ #define CONFIG_BOOTP_DEFAULT (CONFIG_BOOTP_SUBNETMASK | \ CONFIG_BOOTP_GATEWAY | \ CONFIG_BOOTP_HOSTNAME | \
CONFIG_BOOTP_BOOTPATH)
CONFIG_BOOTP_BOOTPATH | \CONFIG_BOOTP_SERVER)#ifndef CONFIG_BOOTP_MASK #define CONFIG_BOOTP_MASK CONFIG_BOOTP_DEFAULT
We don't even have include/cmd_confdefs.h any more...
If I understand corrrectly, this patch changes the behaviour for all boards that currently don't define CONFIG_BOOTP_SERVER.
Is this intentional?
If I understand this correctly, because CONFIG_BOOTP_MASK is tested and I don't see anyone defining this, it will *not* touch any configuration right now. Subsequently for consistency the code in bootp.c _may_ move to also using this MASK variable but then many configs will have to be touched. Many configs potentially can simply move to CONFIG_BOOTP_DEFAULT.
Hmmm... I have to admit that I never liked this CONFIG_BOOTP_MASK stuff so I'm happy it's gone.
So in any case the patch must be cleaned up and resubmitted.
Best regards,
Wolfgang Denk
On Fri, 2007-08-10 at 07:54, Wolfgang Denk wrote:
#define CONFIG_BOOTP_DEFAULT (CONFIG_BOOTP_SUBNETMASK | \ CONFIG_BOOTP_GATEWAY | \ CONFIG_BOOTP_HOSTNAME | \
CONFIG_BOOTP_BOOTPATH)
CONFIG_BOOTP_BOOTPATH | \CONFIG_BOOTP_SERVER)#ifndef CONFIG_BOOTP_MASK #define CONFIG_BOOTP_MASK CONFIG_BOOTP_DEFAULT
We don't even have include/cmd_confdefs.h any more...
That is correct!
As you may recall, I had asked him to rewrite his patch against the new CONFIG_CMD_* style that was in -testing specifically to avoid this issue.
If I understand corrrectly, this patch changes the behaviour for all boards that currently don't define CONFIG_BOOTP_SERVER.
Is this intentional?
If I understand this correctly, because CONFIG_BOOTP_MASK is tested and I don't see anyone defining this, it will *not* touch any configuration right now. Subsequently for consistency the code in bootp.c _may_ move to also using this MASK variable but then many configs will have to be touched. Many configs potentially can simply move to CONFIG_BOOTP_DEFAULT.
Hmmm... I have to admit that I never liked this CONFIG_BOOTP_MASK stuff so I'm happy it's gone.
Hi! I can address this issue! :-)
That is correct. It went away with the Command Config rewrite as it was in the same file and style.
So in any case the patch must be cleaned up and resubmitted.
Agreed.
Best regards,
Wolfgang Denk
Thanks, jdl
Dear Wolfgang,
This change would keep things working as they are, but fix the bug where packets are received before the buffer address is set.
Which bug are you talking about? Is this in U-Boot or in Linux?
The bug is in u-boot. it currently starts the ethernet port before the receive buffer address is initialized. this results in receiving packets into memory 0 and overwriting the vector table.
I'll wait to see your response to Havard's email regarding setting up the MAC Address in u-boot.
thanks, wilson
participants (6)
-
Ben Warren -
Detlev Zundel -
Håvard Skinnemoen -
Jon Loeliger -
Wilson Callan -
Wolfgang Denk