[U-Boot-Users] Pull request: u-boot-freebsd
Dear Wolfgang,
This is a re-spin on the already requested pull for the initial implementation of the extended API for standalone apps, that is used for FreeBSD bootstrapping.
kind regards, Rafal
The following changes since commit 41be969f4957115ed7b1fe8b890bfaee99d7a7a2: Wolfgang Denk (1): Release v1.3.1
are available in the git repository at:
git://denx.de/git/u-boot-freebsd.git master
Rafal Jaworowski (5): [Net] Introduce standalone eth_receive() routine [POWERPC] Simplify bd_info struct Globalize envmatch() so it can be accessed from other U-Boot subsystems API for external applications Style cleanups
Makefile | 7 +- api/Makefile | 40 +++ api/README | 55 ++++ api/api.c | 670 ++++++++++++++++++++++++++++++++++++++++++ api/api_net.c | 114 +++++++ api/api_platform.c | 74 +++++ api/api_storage.c | 370 +++++++++++++++++++++++ api_examples/Makefile | 103 +++++++ api_examples/crt0.S | 49 +++ api_examples/demo.c | 294 ++++++++++++++++++ api_examples/glue.c | 460 +++++++++++++++++++++++++++++ api_examples/libgenwrap.c | 96 ++++++ common/cmd_nvedit.c | 5 +- include/api_public.h | 102 +++++++ include/asm-ppc/u-boot.h | 17 +- include/common.h | 3 + include/configs/MPC8555CDS.h | 4 + include/net.h | 1 + lib_ppc/board.c | 5 + net/eth.c | 52 ++++ net/net.c | 6 + 21 files changed, 2512 insertions(+), 15 deletions(-) create mode 100644 api/Makefile create mode 100644 api/README create mode 100644 api/api.c create mode 100644 api/api_net.c create mode 100644 api/api_platform.c create mode 100644 api/api_storage.c create mode 100644 api_examples/Makefile create mode 100644 api_examples/crt0.S create mode 100644 api_examples/demo.c create mode 100644 api_examples/glue.c create mode 100644 api_examples/libgenwrap.c create mode 100644 include/api_public.h
On Mon, 17 Dec 2007 13:06:42 +0100 Rafal Jaworowski raj@semihalf.com wrote:
API for external applications
I'm sorry, but I really hate this stuff.
* What happens if changes to the API is needed? Will be keep adding new "system calls" every time a new limitation with the existing interface is found (like Linux does)? * In other words, do we really _want_ a "stable API"? * What's the rationale for the "syscalls" this patch exports, and are you absolutely sure the prototypes are sane? If you want a stable API, you need to get it right the first time. * Both the API core and the examples are littered with external declarations. Can we please put such things in header files where it belongs? * All syscalls are implemented as vararg functions, so it's difficult to tell what arguments they take and whether or not they are being used correctly from the other side of the "syscall" line. A standard set of wrappers and associated header files would help, of course. * How is this really different from the existing jumptable stuff? It looks like it's just a different set of exported functions. Will the crufty old jumptable interface be removed at some point? Presumably, the new interface is superior, so it should be. Right?
Haavard
Haavard Skinnemoen wrote:
I'm sorry, but I really hate this stuff.
- What happens if changes to the API is needed? Will be keep adding new "system calls" every time a new limitation with the existing interface is found (like Linux does)?
The API is versioned and consumer code is able to verify it. When we need to change/extend it, the version is bumped: that's similar how many other APIs are managed, like UNIX libs.
- In other words, do we really _want_ a "stable API"?
Depending on the definition of "we", yes :-) Besides that we need a stable API for FreeBSD purposes, I'm sure U-Boot as a whole benefits from improvement in this area.
- What's the rationale for the "syscalls" this patch exports, and are you absolutely sure the prototypes are sane? If you want a stable API, you need to get it right the first time.
Per earlier explanation given by Marcel, the main purpose of these 'syscalls' is providing a well defined and stable interface to access services that U-Boot implements like operations on the console, networking, storage etc. The prototypes we have now have been thought out for a while and reviewed, and I think they are sane.
- Both the API core and the examples are littered with external declarations. Can we please put such things in header files where it belongs?
There's a couple of extern declarations that indeed could be placed in a separate header, but it's usually fine balance when to put something into a separate file (and bloat the files structure..), and in this case I decided not to for simplicity. All other externs are for accessing existing U-Boot objects.
- All syscalls are implemented as vararg functions, so it's difficult to tell what arguments they take and whether or not they are being used correctly from the other side of the "syscall" line. A standard set of wrappers and associated header files would help, of course.
There is a pseudo-signature description in the comment for each syscall that was meant to help and document. Also, the helper wrapper you mention is already there: it's the glue layer, which implements front-end conveniency calls the consumer can use, but it's not mandatory and syscall can be invoked directly.
- How is this really different from the existing jumptable stuff? It looks like it's just a different set of exported functions. Will the crufty old jumptable interface be removed at some point? Presumably, the new interface is superior, so it should be. Right?
The new interface is a similar mechanism, but it does not entangle external application with configuration of U-Boot it happens to run upon. It's meant to be more robust and extensible, but it is still initial implementation. This is why it's optional so might be treated as experimental feature right now, but my hope is after U-Boot-FreeBSD integration settles down, the old jumptable approach can be retired (I volunteer to convert legacy standalone U-Boot apps to the new scheme at that point).
Rafal
On Mon, 17 Dec 2007 18:17:35 +0100 Rafal Jaworowski raj@semihalf.com wrote:
Haavard Skinnemoen wrote:
I'm sorry, but I really hate this stuff.
- What happens if changes to the API is needed? Will be keep adding new "system calls" every time a new limitation with the existing interface is found (like Linux does)?
The API is versioned and consumer code is able to verify it. When we need to change/extend it, the version is bumped: that's similar how many other APIs are managed, like UNIX libs.
So old versions of the interface must be kept around in case legacy applications need them (that's how solib versioning works, IIRC.) Or do applications have to anticipate that the interface may change in a future version?
- In other words, do we really _want_ a "stable API"?
Depending on the definition of "we", yes :-) Besides that we need a stable API for FreeBSD purposes, I'm sure U-Boot as a whole benefits from improvement in this area.
Yeah, I understand that you want to use this with FreeBSD somehow. But I've never seen a good answer to _why_ you need to do it this way, apart from something along the lines of "because we can", or "we want to turn u-boot into a full-fledged OS".
- What's the rationale for the "syscalls" this patch exports, and are you absolutely sure the prototypes are sane? If you want a stable API, you need to get it right the first time.
Per earlier explanation given by Marcel, the main purpose of these 'syscalls' is providing a well defined and stable interface to access services that U-Boot implements like operations on the console, networking, storage etc. The prototypes we have now have been thought out for a while and reviewed, and I think they are sane.
Having the boot loader provide "advanced" services to the OS (console, storage, etc.) has been done before. It's called "BIOS", and it absolutely _sucks_. Please don't turn u-boot into a BIOS...
- Both the API core and the examples are littered with external declarations. Can we please put such things in header files where it belongs?
There's a couple of extern declarations that indeed could be placed in a separate header, but it's usually fine balance when to put something into a separate file (and bloat the files structure..), and in this case I decided not to for simplicity. All other externs are for accessing existing U-Boot objects.
"A couple"? There are sh*tloads of them.
As for accessing existing U-Boot objects, that's not an excuse. If a global function is missing a corresponding header declaration, it should be added.
I've been going around trying to get rid of some global definitions and turning them into static ones, assuming that if they don't exist in a header, they aren't used. The problems usually show up a couple of hours into the final compile-test...
- All syscalls are implemented as vararg functions, so it's difficult to tell what arguments they take and whether or not they are being used correctly from the other side of the "syscall" line. A standard set of wrappers and associated header files would help, of course.
There is a pseudo-signature description in the comment for each syscall that was meant to help and document. Also, the helper wrapper you mention is already there: it's the glue layer, which implements front-end conveniency calls the consumer can use, but it's not mandatory and syscall can be invoked directly.
Yeah, but the stubs have no associated header file, so you have to declare them yourself. That's just begging for fun-to-debug problems where the caller and the callee have different opinions about the function signature...even more fun when the problems only show up on certain architectures.
This is a general problem with the existing u-boot code, btw. I don't dare even guess how many hidden bugs there are all around the tree because of function prototype mismatches...
- How is this really different from the existing jumptable stuff? It looks like it's just a different set of exported functions. Will the crufty old jumptable interface be removed at some point? Presumably, the new interface is superior, so it should be. Right?
The new interface is a similar mechanism, but it does not entangle external application with configuration of U-Boot it happens to run upon. It's meant to be more robust and extensible, but it is still initial implementation. This is why it's optional so might be treated as experimental feature right now, but my hope is after U-Boot-FreeBSD integration settles down, the old jumptable approach can be retired (I volunteer to convert legacy standalone U-Boot apps to the new scheme at that point).
Well, if it's going to _replace_ the old interface, it all sounds a bit more reasonable.
Haavard
Haavard Skinnemoen wrote:
- What happens if changes to the API is needed? Will be keep adding new "system calls" every time a new limitation with the existing interface is found (like Linux does)?
The API is versioned and consumer code is able to verify it. When we need to change/extend it, the version is bumped: that's similar how many other APIs are managed, like UNIX libs.
So old versions of the interface must be kept around in case legacy applications need them (that's how solib versioning works, IIRC.) Or do applications have to anticipate that the interface may change in a future version?
If backwards compatibility is required then yes, this would be nice to have, but let's try not to miss the whole picture: with current U-Boot nine (9) standalone very simple programs are shipping, which use the jumptable approach. Why would they suffer with a more generic calling mechanism?
- Both the API core and the examples are littered with external declarations. Can we please put such things in header files where it belongs?
There's a couple of extern declarations that indeed could be placed in a separate header, but it's usually fine balance when to put something into a separate file (and bloat the files structure..), and in this case I decided not to for simplicity. All other externs are for accessing existing U-Boot objects.
"A couple"? There are sh*tloads of them.
As for accessing existing U-Boot objects, that's not an excuse. If a global function is missing a corresponding header declaration, it should be added.
I don't see this a maintenance difficulty, but if you consider this a major obstacle I'll try to improve thier organization.
- All syscalls are implemented as vararg functions, so it's difficult to tell what arguments they take and whether or not they are being used correctly from the other side of the "syscall" line. A standard set of wrappers and associated header files would help, of course.
There is a pseudo-signature description in the comment for each syscall that was meant to help and document. Also, the helper wrapper you mention is already there: it's the glue layer, which implements front-end conveniency calls the consumer can use, but it's not mandatory and syscall can be invoked directly.
Yeah, but the stubs have no associated header file, so you have to declare them yourself. That's just begging for fun-to-debug problems where the caller and the callee have different opinions about the function signature...even more fun when the problems only show up on certain architectures.
That's a valid point, I'll provide a header file with those.
Rafal
Dear Rafal,
in message 47684C6F.8080303@semihalf.com you wrote:
If backwards compatibility is required then yes, this would be nice to have, but let's try not to miss the whole picture: with current U-Boot nine (9) standalone very simple programs are shipping, which use the jumptable approach. Why would they suffer with a more generic calling mechanism?
Please note that more than 9 are shipping; some are just not in the examples directory. See for example the trab_fkt code in board/trab.
And some more are not in the U-Boot tree because they make use of the fact that standalone apps don't need to be under GPL.
But this is not intended to argument with you - I feel you are right. I just wanted to get the facts clear.
Best regards,
Wolfgang Denk
On Tue, 18 Dec 2007 23:40:47 +0100 Rafal Jaworowski raj@semihalf.com wrote:
Haavard Skinnemoen wrote:
- What happens if changes to the API is needed? Will be keep adding new "system calls" every time a new limitation with the existing interface is found (like Linux does)?
The API is versioned and consumer code is able to verify it. When we need to change/extend it, the version is bumped: that's similar how many other APIs are managed, like UNIX libs.
So old versions of the interface must be kept around in case legacy applications need them (that's how solib versioning works, IIRC.) Or do applications have to anticipate that the interface may change in a future version?
If backwards compatibility is required then yes, this would be nice to have, but let's try not to miss the whole picture: with current U-Boot nine (9) standalone very simple programs are shipping, which use the jumptable approach. Why would they suffer with a more generic calling mechanism?
If this has been discussed before, please give me a pointer.
I think saying that "backwards compatibility would be nice to have" oversimplifies how things work in the real world. Backwards compatibility is something you find out that you need _later_ when lots of applications have started using the initial, broken version of the interface.
And I can say right away that there will be problems with the functions you've already exported. get_sys_info(), for example, is absolutely _horrible_. It is not extensible (caller and callee must agree on the size of struct sys_info) and it contains just random, mostly undocumented pieces of information about the system. This is be bdinfo approach to passing system information all over again.
At the very least, please add a size argument.
So I think that before we export a new interface, every single export must be discussed thoroughly. We may be stuck with them once they're exported.
- Both the API core and the examples are littered with external declarations. Can we please put such things in header files where it belongs?
There's a couple of extern declarations that indeed could be placed in a separate header, but it's usually fine balance when to put something into a separate file (and bloat the files structure..), and in this case I decided not to for simplicity. All other externs are for accessing existing U-Boot objects.
"A couple"? There are sh*tloads of them.
As for accessing existing U-Boot objects, that's not an excuse. If a global function is missing a corresponding header declaration, it should be added.
I don't see this a maintenance difficulty, but if you consider this a major obstacle I'll try to improve thier organization.
Yeah, that would be great. Although I guess it wouldn't be fair to place this burden on you -- there are lots of other instances all around the u-boot tree where global functions are defined with no corresponding header declaration.
Yeah, but the stubs have no associated header file, so you have to declare them yourself. That's just begging for fun-to-debug problems where the caller and the callee have different opinions about the function signature...even more fun when the problems only show up on certain architectures.
That's a valid point, I'll provide a header file with those.
Great :)
Haavard
On Dec 17, 2007, at 9:17 AM, Rafal Jaworowski wrote:
- How is this really different from the existing jumptable stuff? It looks like it's just a different set of exported functions. Will the crufty old jumptable interface be removed at some point? Presumably, the new interface is superior, so it should be. Right?
Let me chime in for this, quickly, because we did try to use the jump table and it proved to be useless. As Rafal pointed out, the problem with using the jump table is that: 1) getting to the jump table requires that you know the layout of the global_data structure, because it's at the end. The layout of the structure is highly variable due to the optional fields. As such, in order to use the jump table, you need to configure the "client" program in the same way as U-Boot, just to make it work. This is especially frustrating if the client program itself has, inherently, no configurable options and you need it to run on a couple of flavors of boards that need slightly different U-Boot configurations. 2) The jump table itself is not fixed in that there are optional entries. On top of that, the jump table has no way to account for extensions or growth, because there's no function you can call that gives you a version or some sorts.
The first point can easily be fixed by putting the jump table first or at least at the head of the global_data structure. The second point can partially be fixed by making all entries non-optional and provide stubs in case a function has no implementation in a certain configuration. This however would be against the basic design philosophy of U-Boot, so would by itself raise concerns.
More importantly, the crux of the second point could not be addressed and that's typically where the problems arise going forward. In fact, we knew that we were going to run into problems, because the current jumptable did not have the interfaces we'd like to see and there were even ones we didn't need right from the start, but knew we'd want within a year. And, once this has become a known interface, who knows what people want to do with it?
The bottom line was that while the existing jump table would get us 80% of the way, getting that additional 20% would be impossible without doing exactly something along the lines of what Rafal did.
I fully realize that for developers who's focus is U-Boot, anything that runs on top of U-Boot and that wants something from U-Boot is a burden at times, but it is important to realize that U-Boot is most of the time nothing more than the first step towards a useful machine and that there can be more than 2 steps in getting on OS to run. The ability to make use of the (hardware) knowledge that U-Boot has, and at the same time its ability to abstract it for the next steps, is in my opinion just as important as having its own prompt.
Just my $0.02
On Mon, 17 Dec 2007 10:06:07 -0800 Marcel Moolenaar xcllnt@mac.com wrote:
I fully realize that for developers who's focus is U-Boot, anything that runs on top of U-Boot and that wants something from U-Boot is a burden at times, but it is important to realize that U-Boot is most of the time nothing more than the first step towards a useful machine and that there can be more than 2 steps in getting on OS to run. The ability to make use of the (hardware) knowledge that U-Boot has, and at the same time its ability to abstract it for the next steps, is in my opinion just as important as having its own prompt.
I agree, but I thought exporting u-boot's hardware knowledge was the whole point of the Device Tree (libfdt) stuff?
It's not that I'm really objecting to the new interface -- it does seem to be more well-designed than the old jumptable stuff -- but it's just that I don't understand why you need this functionality in the first place, just to be able to boot an OS. When you have a kernel image as well as possibly other related images loaded into memory, along with a device tree providing detailed information about the hardware, why do you need a syscall interface on top of everything?
Haavard
On Dec 17, 2007, at 10:19 AM, Haavard Skinnemoen wrote:
On Mon, 17 Dec 2007 10:06:07 -0800 Marcel Moolenaar xcllnt@mac.com wrote:
I fully realize that for developers who's focus is U-Boot, anything that runs on top of U-Boot and that wants something from U-Boot is a burden at times, but it is important to realize that U-Boot is most of the time nothing more than the first step towards a useful machine and that there can be more than 2 steps in getting on OS to run. The ability to make use of the (hardware) knowledge that U-Boot has, and at the same time its ability to abstract it for the next steps, is in my opinion just as important as having its own prompt.
I agree, but I thought exporting u-boot's hardware knowledge was the whole point of the Device Tree (libfdt) stuff?
It's not that I'm really objecting to the new interface -- it does seem to be more well-designed than the old jumptable stuff -- but it's just that I don't understand why you need this functionality in the first place, just to be able to boot an OS. When you have a kernel image as well as possibly other related images loaded into memory, along with a device tree providing detailed information about the hardware, why do you need a syscall interface on top of everything?
[apologies: I appear to be very verbose this monday morning...]
First of all: It is possible to create a FreeBSD version that can be loaded directly from U-Boot. In fact, that's what we used to do. You don't really need an API in U-Boot to boot an O/S. But, there's a cost. Granted, that cost is not really of any concern to U-Boot, for it's a cost that relates to FreeBSD and it's users, but the cost is there nonetheless.
The FreeBSD loader is in a way an intrinsic part of the FreeBSD O/S in that we depend on the loader for module support, as well as the support for memory disks and we interact with the loader be setting options in files under /boot that the loader uses. Without the FreeBSD loader there's no "module" that creates the metadata used by the FreeBSD kernel to find loaded modules and file systems. This means, in practice, that if you eliminate the FreeBSD loader, you drop support for modules and kluge the kernel to allow for a single memory-based root file system and you tell the kernel where it is loaded by using an environment variable of some sort.
Eh, hang on... U-Boot environment variables can only be read by using the corresponding function from the jump table and as per my previous email, there's no way you can get that without building/configuring a kernel that works with that particular U-Boot copy...
The problem we have by not using a loader is exactly the same, except that it's much harder and much more unwanted to configure a FreeBSD kernel at build time for use on a single U-Boot configuration. We, as in FreeBSD, actually want to build a single kernel that at runtime configures itself according to the hardware it finds itself running on and the FreeBSD loader plays a pivotal role in that desire.
Even while we've not reached our ultimate goal in that respect, we know we will always have to build a loader that matches the platform. We will end up having a loader that works within Open Firmware and we will have a loader for U-Boot (and I'm talking PowerPC based H/W only at the moment -- our collection of loaders is actually bigger). In fact, without a loader we will not achieve our ultimate goal of building, say, a single PowerPC kernel that in unison with the loader runs on, say, AIM hardware and Book-E embedded systems.
Now, let's get to a lower level: we also like to be able to build a single, say, PowerPC-based U-Boot loader that works on any PowerPC hardware that uses U-Boot. We know by looking at U-Boot that that won't be possible if we don't have a layer of abstraction somewhere. And unfortunately for U-Boot, that layer of abstraction is ipso-facto provided by the one component that cannot work with an abstraction of the hardware itself. That is the most ideal and logical software stacking design possible. And that's why we need the API in U-Boot.
So where does the device tree come into play? It doesn't really. All this is about basic functionality to have a 2nd stage loader use the console without having to figure out what all the possible consoles could be and which one of those is being selected in U-Boot. It's about having the 2nd stage loader TFTP a kernel, module, file system, etc by using the network drivers in U-Boot. Likewise, it's about getting a kernel from an USB device without having to have another USB driver when the one in U-Boot just works.
A 2nd stage loader does not need to know about the hardware. It just needs some support for networking and/or disk I/O. The kernel can figure it out by itself. U-Boot is not likely to give the right information anyway, because U-Boot may not even know about all the hardware. A PCI bus scan can be done anywhere. USB discovery needs to be in the kernel anyway. Embedded devices... Ah yes, I can see how U-Boot, being configured for the board, knows about an embedded device that is otherwise not (self-)enumerable. This is where I think the stable API will be extended in the future. In some form. In some shape. Maybe a device tree, maybe not...
On Mon, 17 Dec 2007 11:10:59 -0800 Marcel Moolenaar xcllnt@mac.com wrote:
It's not that I'm really objecting to the new interface -- it does seem to be more well-designed than the old jumptable stuff -- but it's just that I don't understand why you need this functionality in the first place, just to be able to boot an OS. When you have a kernel image as well as possibly other related images loaded into memory, along with a device tree providing detailed information about the hardware, why do you need a syscall interface on top of everything?
[apologies: I appear to be very verbose this monday morning...]
That makes two of us, doesn't it? ;-)
<snip lots of useful information about how the FreeBSD loader works>
Thanks for explaining this. I think I understand better why this new interface is needed now.
However, I think this sort of information would be very useful to have in the changelog -- adding a new external interface is not something that should be done lightly, and a good explanation of _why_ the new interface is needed is IMO very important to have on the record.
Haavard
Haavard Skinnemoen wrote:
I fully realize that for developers who's focus is U-Boot, anything that runs on top of U-Boot and that wants something from U-Boot is a burden at times, but it is important to realize that U-Boot is most of the time nothing more than the first step towards a useful machine and that there can be more than 2 steps in getting on OS to run. The ability to make use of the (hardware) knowledge that U-Boot has, and at the same time its ability to abstract it for the next steps, is in my opinion just as important as having its own prompt.
I agree, but I thought exporting u-boot's hardware knowledge was the whole point of the Device Tree (libfdt) stuff?
Well, not all U-Boot platforms use the device tree abstraction: it's only PowerPC+Linux combination. MIPS, ARM and other archs don't use it, operating systems other than Linux, to name only *BSD and VxWorks, similar.
It's not that I'm really objecting to the new interface -- it does seem to be more well-designed than the old jumptable stuff -- but it's just that I don't understand why you need this functionality in the first place, just to be able to boot an OS. When you have a kernel image as well as possibly other related images loaded into memory, along with a device tree providing detailed information about the hardware, why do you need a syscall interface on top of everything?
Let me try to summarize the FreeBSD booting approach:
1. FreeBSD has it's own last stage loader(8)
- it prepares the booting environment for kernel boostrap (the so called metadata, which mainly inlude kernel env vars, boot flags like single user, verbose mode and so on)
- it allows to load and unload dynamic kernel modules at this early stage (even the kernel ELF is treated equally as a big dynamic module itself), set certain tunables before bootup
- it lets you have menus, startup scripts and similar facilities
- it knows filesystems (UFS, ISO9660 etc.) to retrieve kernel and its modules from
- it's mainly architecture independent and relies on the underlying firmware to do elementary operations on the console, networking (send and receive frame), disk (read, write block) etc. In i386 world this is BIOS, on ia64 EFI, on embedded PowerPC (us :) it is U-Boot and so on
- it provides uniform UI, behaviour and those above features accross all architectures which FreeBSD supports
2. In order to provide full featured FreeBSD port we brought the loader(8) for the embedded PowerPC: it runs as a standalone app on U-Boot, and in turn kicks off the kernel, giving it all native FreeBSD environment described in p.1
Hope that helped explain the scenario a bit.
Rafal
On Wed, 19 Dec 2007 00:00:09 +0100 Rafal Jaworowski raj@semihalf.com wrote:
Haavard Skinnemoen wrote:
I agree, but I thought exporting u-boot's hardware knowledge was the whole point of the Device Tree (libfdt) stuff?
Well, not all U-Boot platforms use the device tree abstraction: it's only PowerPC+Linux combination. MIPS, ARM and other archs don't use it, operating systems other than Linux, to name only *BSD and VxWorks, similar.
True, but wouldn't it make more sense to try to unify this rather than adding another interface?
It's not that I'm really objecting to the new interface -- it does seem to be more well-designed than the old jumptable stuff -- but it's just that I don't understand why you need this functionality in the first place, just to be able to boot an OS. When you have a kernel image as well as possibly other related images loaded into memory, along with a device tree providing detailed information about the hardware, why do you need a syscall interface on top of everything?
Let me try to summarize the FreeBSD booting approach:
<snip>
Hope that helped explain the scenario a bit.
It did. Thanks.
Haavard
Dear Rafal,
in message 47666652.30600@semihalf.com you wrote:
This is a re-spin on the already requested pull for the initial implementation of the extended API for standalone apps, that is used for FreeBSD bootstrapping.
I'm afraid I have to reject this pull request.
First, I think it breaks Linux booting support for a couple of boards (all 4xx ones, that is).
The following changes since commit 41be969f4957115ed7b1fe8b890bfaee99d7a7a2: Wolfgang Denk (1): Release v1.3.1
are available in the git repository at:
...
[POWERPC] Simplify bd_info struct
This is the culprit.
With your patch, you *always* include an entry "unsigned long bi_bar;" in the bd_infor structure - even for processors which didn't have one before (like 4xx boards). Thus you change the layout of the bd_info structure for such systems, which causes an incompatibility with the respective structure used by the Linux kernel.
Also, I have to admit that I dislike this type of #ifdef based "simplification". I don't think the resulting code becomes more readable.
Please fix and resubmit.
Then, we have this part:
[Net] Introduce standalone eth_receive() routine
This adds a lot of code to the networking code which is not neede dby most of the boards. Please make this an optional feature that get's only compiled in for boards that explicitely request it. Then run this patch separately through the network custodian.
Thanks.
Best regards,
Wolfgang Denk
Dear Wolfgang, you wrote:
I'm afraid I have to reject this pull request.
First, I think it breaks Linux booting support for a couple of boards (all 4xx ones, that is).
The following changes since commit 41be969f4957115ed7b1fe8b890bfaee99d7a7a2: Wolfgang Denk (1): Release v1.3.1
are available in the git repository at:
...
[POWERPC] Simplify bd_info structThis is the culprit.
With your patch, you *always* include an entry "unsigned long bi_bar;" in the bd_infor structure - even for processors which didn't have one before (like 4xx boards). Thus you change the layout of the bd_info structure for such systems, which causes an incompatibility with the respective structure used by the Linux kernel.
Also, I have to admit that I dislike this type of #ifdef based "simplification". I don't think the resulting code becomes more readable.
Mhm, right. Maybe it was too much of an improvement :) Given all the legacy dependencies between bd_info and Linux it's probably better to just leave it as is and deal with those #idef'ed fields locally (where they are accessed).
Please fix and resubmit.
Will do.
Then, we have this part:
[Net] Introduce standalone eth_receive() routineThis adds a lot of code to the networking code which is not neede dby most of the boards. Please make this an optional feature that get's only compiled in for boards that explicitely request it. Then run this patch separately through the network custodian.
I already sent this patch to Ben and the list some time ago and got an initial ACK, but I'll re-spin with.
kind regards, Rafal
Hi Rafal,
Rafal Jaworowski wrote:
Dear Wolfgang, you wrote:
I'm afraid I have to reject this pull request.
First, I think it breaks Linux booting support for a couple of boards (all 4xx ones, that is).
The following changes since commit 41be969f4957115ed7b1fe8b890bfaee99d7a7a2: Wolfgang Denk (1): Release v1.3.1
are available in the git repository at:
...
[POWERPC] Simplify bd_info structThis is the culprit.
With your patch, you *always* include an entry "unsigned long bi_bar;" in the bd_infor structure - even for processors which didn't have one before (like 4xx boards). Thus you change the layout of the bd_info structure for such systems, which causes an incompatibility with the respective structure used by the Linux kernel.
Also, I have to admit that I dislike this type of #ifdef based "simplification". I don't think the resulting code becomes more readable.
Mhm, right. Maybe it was too much of an improvement :) Given all the legacy dependencies between bd_info and Linux it's probably better to just leave it as is and deal with those #idef'ed fields locally (where they are accessed).
Please fix and resubmit.
Will do.
Then, we have this part:
[Net] Introduce standalone eth_receive() routineThis adds a lot of code to the networking code which is not neede dby most of the boards. Please make this an optional feature that get's only compiled in for boards that explicitely request it. Then run this patch separately through the network custodian.
I already sent this patch to Ben and the list some time ago and got an initial ACK, but I'll re-spin with.
When you send the re-spun patch I'll be sure to pull it in.
regards, Ben
Hi Ben,
On Thu, Dec 27, 2007 at 11:27:50AM -0500, Ben Warren wrote:
[Net] Introduce standalone eth_receive() routineThis adds a lot of code to the networking code which is not neede dby most of the boards. Please make this an optional feature that get's only compiled in for boards that explicitely request it. Then run this patch separately through the network custodian.
I already sent this patch to Ben and the list some time ago and got an initial ACK, but I'll re-spin with.
When you send the re-spun patch I'll be sure to pull it in.
The patch is included below. I'm only wondering if it makes sense to run this patch through you and the rest of my changes separately, while they all belong to the same changeset and kind of depend on this one...
kind regards, Rafal
[PATCH] [Net] Introduce standalone eth_receive() routine.
The purpose of this routine is receiving a single network frame, outside of U-Boot's NetLoop(). Exporting it to standalone programs that run on top of U-Boot will let them utilise networking facilities. For sending a raw frame the already existing eth_send() can be used.
The direct consumer of this routine is the newly introduced API layer for external applications (enabled with CONFIG_API).
Signed-off-by: Rafal Jaworowski raj@semihalf.com Signed-off-by: Piotr Kruszynski ppk@semihalf.com --- include/net.h | 3 ++ net/eth.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ net/net.c | 10 +++++++++ 3 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/include/net.h b/include/net.h index 603452a..f6decdc 100644 --- a/include/net.h +++ b/include/net.h @@ -122,6 +122,9 @@ extern void eth_set_enetaddr(int num, char* a); /* Set new MAC address */
extern int eth_init(bd_t *bis); /* Initialize the device */ extern int eth_send(volatile void *packet, int length); /* Send a packet */ +#ifdef CONFIG_API +extern int eth_receive(volatile void *packet, int length); /* Receive a packet */ +#endif extern int eth_rx(void); /* Check for received packets */ extern void eth_halt(void); /* stop SCC */ extern char *eth_get_name(void); /* get name of current device */ diff --git a/net/eth.c b/net/eth.c index 1b56a35..4657f79 100644 --- a/net/eth.c +++ b/net/eth.c @@ -62,6 +62,17 @@ extern int bfin_EMAC_initialize(bd_t *); extern int atstk1000_eth_initialize(bd_t *); extern int mcffec_initialize(bd_t*);
+#ifdef CONFIG_API +extern void (*push_packet)(volatile void *, int); + +static struct { + uchar data[PKTSIZE]; + int length; +} eth_rcv_bufs[PKTBUFSRX]; + +static unsigned int eth_rcv_current = 0, eth_rcv_last = 0; +#endif + static struct eth_device *eth_devices, *eth_current;
struct eth_device *eth_get_dev(void) @@ -457,6 +468,53 @@ int eth_rx(void) return eth_current->recv(eth_current); }
+#ifdef CONFIG_API +static void eth_save_packet(volatile void *packet, int length) +{ + volatile char *p = packet; + int i; + + if ((eth_rcv_last+1) % PKTBUFSRX == eth_rcv_current) + return; + + if (PKTSIZE < length) + return; + + for (i = 0; i < length; i++) + eth_rcv_bufs[eth_rcv_last].data[i] = p[i]; + + eth_rcv_bufs[eth_rcv_last].length = length; + eth_rcv_last = (eth_rcv_last + 1) % PKTBUFSRX; +} + +int eth_receive(volatile void *packet, int length) +{ + volatile char *p = packet; + void *pp = push_packet; + int i; + + if (eth_rcv_current == eth_rcv_last) { + push_packet = eth_save_packet; + eth_rx(); + push_packet = pp; + + if (eth_rcv_current == eth_rcv_last) + return -1; + } + + if (length < eth_rcv_bufs[eth_rcv_current].length) + return -1; + + length = eth_rcv_bufs[eth_rcv_current].length; + + for (i = 0; i < length; i++) + p[i] = eth_rcv_bufs[eth_rcv_current].data[i]; + + eth_rcv_current = (eth_rcv_current + 1) % PKTBUFSRX; + return length; +} +#endif /* CONFIG_API */ + void eth_try_another(int first_restart) { static struct eth_device *first_failed = NULL; diff --git a/net/net.c b/net/net.c index c719bc4..cac3e09 100644 --- a/net/net.c +++ b/net/net.c @@ -137,6 +137,9 @@ uchar NetBcastAddr[6] = /* Ethernet bcast address */ { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; uchar NetEtherNullAddr[6] = { 0, 0, 0, 0, 0, 0 }; +#ifdef CONFIG_API +void (*push_packet)(volatile void *, int len) = 0; +#endif #if defined(CONFIG_CMD_CDP) uchar NetCDPAddr[6] = /* Ethernet bcast address */ { 0x01, 0x00, 0x0c, 0xcc, 0xcc, 0xcc }; @@ -1161,6 +1164,13 @@ NetReceive(volatile uchar * inpkt, int len) if (len < ETHER_HDR_SIZE) return;
+#ifdef CONFIG_API + if (push_packet) { + (*push_packet)(inpkt, len); + return; + } +#endif + #if defined(CONFIG_CMD_CDP) /* keep track if packet is CDP */ iscdp = memcmp(et->et_dest, NetCDPAddr, 6) == 0;
Dear Rafal,
in message 4773930B.9070903@semihalf.com you wrote:
Mhm, right. Maybe it was too much of an improvement :) Given all the legacy dependencies between bd_info and Linux it's probably better to just leave it as is and deal with those #idef'ed fields locally (where they are accessed).
Agreed.
This adds a lot of code to the networking code which is not neede dby most of the boards. Please make this an optional feature that get's only compiled in for boards that explicitely request it. Then run this patch separately through the network custodian.
I already sent this patch to Ben and the list some time ago and got an initial ACK, but I'll re-spin with.
Even is Ben ACKed this patch, it should go through *his* repository, not your's (which is supposed to contain FreeBSD related changes only).
Best regards,
Wolfgang Denk
Dear Wolfgang,
you wrote:
This adds a lot of code to the networking code which is not neede dby most of the boards. Please make this an optional feature that get's only compiled in for boards that explicitely request it. Then run this patch separately through the network custodian.
I already sent this patch to Ben and the list some time ago and got an initial ACK, but I'll re-spin with.
Even is Ben ACKed this patch, it should go through *his* repository, not your's (which is supposed to contain FreeBSD related changes only).
Yes, I know what you mean, although it's not always that sharp in case of cross-functional areas changes. Also, assuming such purely orthogonal development model may bring unnecessary burden and delays when a greater number of areas (and custodians) is involved: individual changes need to be processed by responsible custodians, merging should happen in order and so on. Just my thoughts :) But I can wait for Ben, no problem.
kind regards, Rafal
Rafal Jaworowski wrote:
Dear Wolfgang,
you wrote:
This adds a lot of code to the networking code which is not neede dby most of the boards. Please make this an optional feature that get's only compiled in for boards that explicitely request it. Then run this patch separately through the network custodian.
I already sent this patch to Ben and the list some time ago and got an initial ACK, but I'll re-spin with.
Even is Ben ACKed this patch, it should go through *his* repository, not your's (which is supposed to contain FreeBSD related changes only).
Yes, I know what you mean, although it's not always that sharp in case of cross-functional areas changes. Also, assuming such purely orthogonal development model may bring unnecessary burden and delays when a greater number of areas (and custodians) is involved: individual changes need to be processed by responsible custodians, merging should happen in order and so on. Just my thoughts :) But I can wait for Ben, no problem.
Yeah, I don't know which is the best way to handle this case, and you're right that it's certainly not unique. I can either take the one patch, or you can add my signed-off-by. As grand poo-bah, maybe Wolfgang can set policy here...
regards, Ben
In message 4773E10D.3040007@semihalf.com you wrote:
Even is Ben ACKed this patch, it should go through *his* repository, not your's (which is supposed to contain FreeBSD related changes only).
Yes, I know what you mean, although it's not always that sharp in case of cross-functional areas changes. Also, assuming such purely orthogonal development model may bring unnecessary burden and delays when a greater number of areas (and custodians) is involved: individual changes need to be processed by responsible custodians, merging should happen in order and so on. Just my thoughts :) But I can wait for Ben, no problem.
Yes, I know what you mean. But the whole concept of having custodians with separate responsibilities kind of requires orthogonality.
I have to admit that I don't have experience dealing with such situations, either. So what I'm trying to do is (once more) to follow the Linux model. When submitting support for a new board (which is a pretty standard task) I would like to be able to submit a single patch to one mailing list myself. Neverless you have to split it up into pieces for, say, linuxppc-dev, lmsensors, mtd, linuxusb, and who knows what else - because responsibilities for these separate parts are distributed. I don't like this myself from the submitter's point of view, but from the maintainer's/custodian's point of view it makes a lot sense.
My $ 0.02...
Best regards,
Wolfgang Denk
participants (6)
-
Ben Warren -
Ben Warren -
Haavard Skinnemoen -
Marcel Moolenaar -
Rafal Jaworowski -
Wolfgang Denk