[U-Boot-Users] New and updated patches sent to U-Boot maintainer
I have submitted four patches against U-Boot to Wolfgang Denk directly due to list attachment size limits.
1. add-xilinx-drivers (new). This patch adds drivers for Xilinx FPGA IP cores to the ./drivers subdirectory in U-Boot. Now all boards using processors contained in Xilinx Virtex FPGAs can use a common set of drivers. All drivers use GPLed code taken from either the MontaVista Linux kernel sources or U-Boot itself. Drivers have been tested with the ANT2 and memec_2vpxx boards.
2. add-ant2-bsp (update). This patch updates my original patch of the same name to use the new Xilinx driver libraries. The patch is much smaller now.
3. add-memec_2vpxx-bsp (update). This patch updates my original patch of the same name to use the Xilinx driver libraries. Patch is also much smaller now.
4. add-env-in-sysace-update (updated). This patch updates the original patch by adding a configuration option to use the Xilinx SystemAce driver library to read/write sectors to the CompactFlash card.
Regards, Keith Outwater
Dear Keith,
in message OF0EE85153.67A4AEC8-ON072571E6.005FB093-072571E6.006006D2@mck.us.ray.com you wrote:
I have submitted four patches against U-Boot to Wolfgang Denk directly due to list attachment size limits.
Can you please update your patches using a current code base? I get a lot of rejects when applying your patches.
Also, please perform the following cleanup / changes:
* Make sure to adapt the Makefiles to the new build environment extensions (BUILD_DIR and depend).
* Please create just a single 'drivers/xilinx' directory instead of 5; use subdirectories where really necessary
* Clean up the Coding style (trailing white space in 65 files, C++ comments, indentation not by tabs, trailing empty lines, etc.)
* Please add correct author information; "Author: Xilinx, Inc." is not acceptable.
* Please clean up your license headers. The GPL is pretty clear that there must not be any additional restrictions on the code. Combining this with statements like "YOU ARE RESPONSIBLE FOR OBTAINING ANY THIRD PARTY RIGHTS YOU MAY REQUIRE" is IMHO not acceptable. Either this is GPL, and I have all rights, or it isn't. Also, "Use in such applications is expressly prohibited." is a restriction that is IMO incompatible with the GPL.
* Please clean up the function headers and comments. Something like this:
126 /*****************************************************************************/ 127 /** 128 * 129 * Sets up a callback function to be invoked when an assert occurs. If there 130 * was already a callback installed, then it is replaced. 131 * 132 * @param Routine is the callback to be invoked when an assert is taken 133 * 134 * @return 135 * 136 * None. 137 * 138 * @note 139 * 140 * This function has no effect if NDEBUG is set 141 * 142 ******************************************************************************/ 143 void 144 XAssertSetCallback(XAssertCallback Routine) 145 { 146 XAssertCallbackRoutine = Routine; 147 }
(i. e. 17 lines of mostly empty comment versus 1 line of code) is not acceptable. Please be descriptive, but terse.
This code is *terrybly* bloated - so much, that it's actually unreadable.
Is there any chance to clean this up such that only the *necessary* stuff gets included? You don;t want to tell me that all this is needed or even used?
Please resubmit after cleanup. Thanks.
Best regards,
Wolfgang Denk
Dear Keith,
in message
OF0EE85153.67A4AEC8-ON072571E6.005FB093-072571E6.006006D2@mck.us.ray.com you wrote:
I have submitted four patches against U-Boot to Wolfgang Denk directly
due
to list attachment size limits.
Can you please update your patches using a current code base? I get a lot of rejects when applying your patches.
OK. I was hoping I could get away with not having to do that. Sorry!
Also, please perform the following cleanup / changes:
- Make sure to adapt the Makefiles to the new build environment extensions (BUILD_DIR and depend).
OK
- Please create just a single 'drivers/xilinx' directory instead of 5; use subdirectories where really necessary
OK. I should have done this the first time.
- Clean up the Coding style (trailing white space in 65 files, C++ comments, indentation not by tabs, trailing empty lines, etc.)
I'll run all the sources through GNU indent.
Please add correct author information; "Author: Xilinx, Inc." is not acceptable.
Please clean up your license headers. The GPL is pretty clear that there must not be any additional restrictions on the code. Combining this with statements like "YOU ARE RESPONSIBLE FOR OBTAINING ANY THIRD PARTY RIGHTS YOU MAY REQUIRE" is IMHO not acceptable. Either this is GPL, and I have all rights, or it isn't. Also, "Use in such applications is expressly prohibited." is a restriction that is IMO incompatible with the GPL.
Perhaps the current git head does not have these statements in the Xilinx code in ./board/xilinx/xilinx_enet, for example. I'll check tonight. My source base does have them, though, but my code is a little old.
- Please clean up the function headers and comments. Something like
this:
126
/*****************************************************************************/
127 /** 128 * 129 * Sets up a callback function to be invoked when an assert
occurs. If there
130 * was already a callback installed, then it is replaced. 131 * 132 * @param Routine is the callback to be invoked when an assert
is taken
133 * 134 * @return 135 * 136 * None. 137 * 138 * @note 139 * 140 * This function has no effect if NDEBUG is set 141 * 142
******************************************************************************/
143 void 144 XAssertSetCallback(XAssertCallback Routine) 145 { 146 XAssertCallbackRoutine = Routine; 147 }(i. e. 17 lines of mostly empty comment versus 1 line of code) is not acceptable. Please be descriptive, but terse.
This code is *terrybly* bloated - so much, that it's actually unreadable.
I tend to agree, but this is the Xilinx provided driver code. I can take not credit for it's quality :)
BTW, Some of this code is already in U-Boot (./board/xilinx/common ./board/xilinx_emac ./board/xilinx_iic). My plan was to put it into a common location after which anyone else using the code in ./boards/xilinx could switch over to the code in ./drivers
Is there any chance to clean this up such that only the *necessary* stuff gets included? You don;t want to tell me that all this is needed or even used?
The idea was to use the driver source code as-is to avoid having to re-invent the wheel. I would expect that as more Xilinx FPGA based boards are added, the percentage use of the Xilinx driver code would increase. I actually do use a lot of the functionality provided by the drivers because one of my primary uses of U-Boot is to assist in hardware design verification and debug. So I end up having to write lots of diagnostic code to examine hardware when, for example, I am trying to get the Xilinx SystemAce to work properly. Also, the systems I work on do not tend to be resource constrained. A luxury, to be sure, but it means I can tolerate larger executables.
Please resubmit after cleanup. Thanks.
OK.
Best regards,
Wolfgang Denk
-- Software Engineering: Embedded and Realtime Systems, Embedded Linux Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "The bad reputation UNIX has gotten is totally undeserved, laid on by people who don't understand, who have not gotten in there and tried anything." -- Jim Joyce, owner of Jim Joyce's UNIX Bookstore
Dear Keith,
in message OF30CE6C3A.DAA104F1-ON072571E6.008079A0-072571E6.00823609@mck.us.ray.com you wrote:
Can you please update your patches using a current code base? I get a lot of rejects when applying your patches.
OK. I was hoping I could get away with not having to do that. Sorry!
Ummm... no. If new submitted patches throw too many and too big rejects I will not spend the effort for you.
- Clean up the Coding style (trailing white space in 65 files, C++ comments, indentation not by tabs, trailing empty lines, etc.)
I'll run all the sources through GNU indent.
I'm not sure if this is a good idea.
Please see the "Coding Standards" section in the README.
Perhaps the current git head does not have these statements in the Xilinx code in ./board/xilinx/xilinx_enet, for example. I'll check tonight. My source base does have them, though, but my code is a little old.
Please don't refer to the existing xilinx code as an example - it contains enough of problems.
- Please clean up the function headers and comments. Something like
this:
...
I tend to agree, but this is the Xilinx provided driver code. I can take not credit for it's quality :)
But you submit it for U-Boot. The Linux kernel folks have a long tradition of rejecting poor code, and I am tempted to do the same here.
BTW, Some of this code is already in U-Boot
Yes, and I am angry with myself that I didn't reject this when it was submitted. It just escaped my attention.
My plan was to put it into a common location after which anyone else using the code in ./boards/xilinx could switch over to the code in ./drivers
Is this the only change, i. e. no improvement in the quality of the code?
The idea was to use the driver source code as-is to avoid having to re-invent the wheel. I would expect that as more Xilinx FPGA based boards are added, the percentage use of the Xilinx driver code would increase.
We just discussed this in another case: I don;t want to have dead code in U-Boot, and I guess that 90% of what you're adding here is just code bloat and things that are not needed or referenced by any of the supported boards. But given the poor quality of the code it's time-consuming to check even this.
I'd really appreciate if you could spend some efforts on cleaning this up and bringing it into a more readable / usable form.
Thanks in advance.
Best regards,
Wolfgang Denk
On Mon, 11 Sep 2006, Keith J Outwater wrote: . . .
- add-xilinx-drivers (new). This patch adds drivers for Xilinx FPGA IP
cores to the ./drivers subdirectory in U-Boot. Now all boards using processors contained in Xilinx Virtex FPGAs can use a common set of drivers. All drivers use GPLed code taken from either the MontaVista Linux kernel sources or U-Boot itself. Drivers have been tested with the ANT2 and memec_2vpxx boards.
If you're willing, given that this is FPGA-based I think it would be wise to document not only the boards (physical hardware) that are supported but also the specific versions of FPGA-based "hardware", e.g. "PLB EMAC v2.00f". In my experience this has been a problem with things like the Linux ker- nel; there is no indication of what version(s) of FPGA-based hardware a given driver is designed for.
Tom
. . .
- add-xilinx-drivers (new). This patch adds drivers for Xilinx FPGA
IP
cores to the ./drivers subdirectory in U-Boot. Now all boards using processors contained in Xilinx Virtex FPGAs can use a common set of drivers. All drivers use GPLed code taken from either the MontaVista
Linux
kernel sources or U-Boot itself. Drivers have been tested with the
ANT2
and memec_2vpxx boards.
If you're willing, given that this is FPGA-based I think it would be
wise
to document not only the boards (physical hardware) that are supported
but
also the specific versions of FPGA-based "hardware", e.g. "PLB EMAC
v2.00f".
In my experience this has been a problem with things like the Linux ker- nel; there is no indication of what version(s) of FPGA-based hardware a given driver is designed for.
Tom, I wholeheartedly agree. Unfortunately, there does not seem to be a consensus between all of the parties involved as to the correct approach. In the absence of any agreement, I'm at least trying to get the drivers in a common area so that we can get things moving. I'm supporting two Xilinx FPGA designs (one custom, on eval board) and I have a pile of patches that I'd like to get incorporated. Wolfgang has some issues with the Xilinx code that I'm going to try and address, but at some point there is going to be problems with driver versioning as U-Boot is ported to more Xilinx FPGA based boards and as the Xilinx IP cores evolve. I'm interested in getting these things resolved because I'm feeling the pain.
Keith
participants (3)
-
Keith J Outwater -
T Ziomek -
Wolfgang Denk