[U-Boot] common/cmd_disk.c doesn't actually define any "commands"
another observation from my weekend perusal of all of the common/cmd_*.c files is that, despite its "cmd_" filename prefix, the source file cmd_disk.c doesn't define any actual u-boot commands. according to what i see as u-boot filename naming conventions, it shouldn't be named "cmd_*", should it?
rday
Hi Robert,
On Mon, 04 Feb 2013 07:53:43 -0500, "Robert P. J. Day" rpjday@crashcourse.ca wrote:
another observation from my weekend perusal of all of the common/cmd_*.c files is that, despite its "cmd_" filename prefix, the source file cmd_disk.c doesn't define any actual u-boot commands. according to what i see as u-boot filename naming conventions, it shouldn't be named "cmd_*", should it?
That's arguable: apparently it provides common_diskboot() which implements commands for cmd_ide, cmd_scsi, cmd_usb which use it for implementing their do_diskboot, du_scsiboot, do_usbboot commands respectively. It's purely related to cmd_*.
rday
Amicalement,
Quoting Albert ARIBAUD albert.u.boot@aribaud.net:
Hi Robert,
On Mon, 04 Feb 2013 07:53:43 -0500, "Robert P. J. Day" rpjday@crashcourse.ca wrote:
another observation from my weekend perusal of all of the common/cmd_*.c files is that, despite its "cmd_" filename prefix, the source file cmd_disk.c doesn't define any actual u-boot commands. according to what i see as u-boot filename naming conventions, it shouldn't be named "cmd_*", should it?
That's arguable: apparently it provides common_diskboot() which implements commands for cmd_ide, cmd_scsi, cmd_usb which use it for implementing their do_diskboot, du_scsiboot, do_usbboot commands respectively. It's purely related to cmd_*.
just to be clear, i have no strong opinion on this either way, but my understanding is that source files with the name of "common/cmd_*.c" typically define at least one u-boot "command" with the U_BOOT_CMD macro. as far as i can tell, cmd_disk.c is the only counterexample of that.
it may be true that that file provides utility routines for other actual "commmand" files, but so do many others. for example, cmd_fdt.c defines the behaviour of the actual "fdt" command, while fdt-related utility routines are in fdt_support.c or even in libfdt/.
it's no big deal, i'm just pointing out that cmd_disk.c flies in the face of a fairly obvious pattern in u-boot filenaming convention. and on that note, i will shut up about it. :-)
rday
Hi Robert, Albert,
Robert P. J. Day wrote: just to be clear, i have no strong opinion on this either way, but my understanding is that source files with the name of "common/cmd_*.c" typically define at least one u-boot "command" with the U_BOOT_CMD macro. as far as i can tell, cmd_disk.c is the only counterexample of that.
I was just looking into this myself but from another perspective. I'm using "CONFIG_USB:STORAGE" and because of the way things are made, I then automatically get "usbboot" for which I have no use. Things would be more logical if it was up to cmd_disk to define the commands based on ifdef's for the various backends -- but ONLY if CONFIG_CMD_DISK was in the config file.
Maybe this upsets a lot of other people though...?
Best regards, Mats
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
i know i mentioned it this weekend but the only reason i tripped over that cmd_disk.c thingie is that i built myself a cmd_*.c reference list, matching source files with defined commands and any preprocessor conditions that defined them here:
http://www.crashcourse.ca/wiki/index.php/U-Boot_command/file_reference
i just wanted a truly up-to-date list of u-boot commands, where they come from, and any CONFIG_ settings necessary for compiling them into the u-boot image. it was, unsurprisingly, a very educational experience, where i wandered across the occasional oddity.
rday
Hi Mats,
On Mon, 4 Feb 2013 15:17:17 +0000, Mats Kärrman Mats.Karrman@tritech.se wrote:
Hi Robert, Albert,
Robert P. J. Day wrote: just to be clear, i have no strong opinion on this either way, but my understanding is that source files with the name of "common/cmd_*.c" typically define at least one u-boot "command" with the U_BOOT_CMD macro. as far as i can tell, cmd_disk.c is the only counterexample of that.
I was just looking into this myself but from another perspective. I'm using "CONFIG_USB:STORAGE" and because of the way things are made, I then automatically get "usbboot" for which I have no use. Things would be more logical if it was up to cmd_disk to define the commands based on ifdef's for the various backends -- but ONLY if CONFIG_CMD_DISK was in the config file.
Maybe this upsets a lot of other people though...?
Maybe, but the problem you state is not about cmd_disk (or I am missing the point). USB commands are in USB related files, e.g. do_usbboot() is in cmd_usb.c, so that's where a conditional should be put if you want to compile the command out, rather than in cmd_disk, which does not add to the U-Boot commands table at all.
Best regards, Mats
Amicalement,
Hi Albert,
Albert ARIBAUD wrote:
Maybe, but the problem you state is not about cmd_disk (or I am missing the point). USB commands are in USB related files, e.g. do_usbboot() is in cmd_usb.c, so that's where a conditional should be put if you want to compile the command out, rather than in cmd_disk, which does not add to the U-Boot commands table at all.
The cmd_usb file contains all the other USB commands through "do_usb" and "usbboot" through "do_usbboot" that is just a forward to "common_diskboot". Maybe the major miss-feature here is that you get usbboot and a bunch of extra code, just because you want to be able to read USB memories. This could of course be fixed by revising the ifdefs in cmd_usb etc. instead but in that case I support Robert in his remark about the file naming ;)
BR // Mats
Hi Mats,
On Mon, 4 Feb 2013 15:42:17 +0000, Mats Kärrman Mats.Karrman@tritech.se wrote:
Hi Albert,
Albert ARIBAUD wrote:
Maybe, but the problem you state is not about cmd_disk (or I am missing the point). USB commands are in USB related files, e.g. do_usbboot() is in cmd_usb.c, so that's where a conditional should be put if you want to compile the command out, rather than in cmd_disk, which does not add to the U-Boot commands table at all.
The cmd_usb file contains all the other USB commands through "do_usb" and "usbboot" through "do_usbboot" that is just a forward to "common_diskboot". Maybe the major miss-feature here is that you get usbboot and a bunch of extra code, just because you want to be able to read USB memories. This could of course be fixed by revising the ifdefs in cmd_usb etc. instead but in that case I support Robert in his remark about the file naming ;)
Actually there is no way to fulfill your need to make the usbboot command conditionally compiled by modifying cmd_disk, because cmd_disk simply does not define any command -- so you will have to put the conditionals in cmd_usb.c no matter what.
Now you may want to also conditionally compile cmd_disk.c only of USB, SCSI or IDE require it, but this you can and should do in the Makefile; remember cmd_disk.c is only useful to provide common_diskboot(), so either you completely compile it, or you don't compile it at all.
(now this could be different if we use gcc's -fdata-sections, -ffunction-sections along with ld's --gc-sections, as then we could argue that even if compiled, cmd_disk.c would be linked out if unreferenced.)
As for the name, Robert's issue stems from his assumption that a file with cmd_ necessarily declares a listable U-Boot command. I assume that files with cmd_ contain command-related code not necessarily including a listable command -- for instance here, a command execution function which is mapped to a (set of three) listable command(s) elsewhere.
BR // Mats
Amicalement,
Hi Robert,
On Mon, 04 Feb 2013 10:04:20 -0500, "Robert P. J. Day" rpjday@crashcourse.ca wrote:
Quoting Albert ARIBAUD albert.u.boot@aribaud.net:
Hi Robert,
On Mon, 04 Feb 2013 07:53:43 -0500, "Robert P. J. Day" rpjday@crashcourse.ca wrote:
another observation from my weekend perusal of all of the common/cmd_*.c files is that, despite its "cmd_" filename prefix, the source file cmd_disk.c doesn't define any actual u-boot commands. according to what i see as u-boot filename naming conventions, it shouldn't be named "cmd_*", should it?
That's arguable: apparently it provides common_diskboot() which implements commands for cmd_ide, cmd_scsi, cmd_usb which use it for implementing their do_diskboot, du_scsiboot, do_usbboot commands respectively. It's purely related to cmd_*.
just to be clear, i have no strong opinion on this either way, but my understanding is that source files with the name of "common/cmd_*.c" typically define at least one u-boot "command" with the U_BOOT_CMD macro. as far as i can tell, cmd_disk.c is the only counterexample of that.
it may be true that that file provides utility routines for other actual "commmand" files, but so do many others. for example, cmd_fdt.c defines the behaviour of the actual "fdt" command, while fdt-related utility routines are in fdt_support.c or even in libfdt/.
That's understandable as libfdt provides support for much more than commands; OTOH, cmd_disk provides support *only* to commands.
it's no big deal, i'm just pointing out that cmd_disk.c flies in the face of a fairly obvious pattern in u-boot filenaming convention. and on that note, i will shut up about it. :-)
rday
Amicalement,
participants (3)
-
Albert ARIBAUD -
Mats Kärrman -
Robert P. J. Day