| Message ID | 4CFD863A.7070000@mentor.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Marek Vasut |
| Headers | show |
Dear Deepak Saxena, In message <4CFD863A.7070000@mentor.com> you wrote: > commit 341764495180a712b9aaccfa0479b2ff7e44e35b > Author: Deepak Saxena <deepak_saxena@mentor.com> > Date: Mon Dec 6 15:52:07 2010 -0800 > > Honor /memory/reg node in DTB files > > This patch adds code to the bootm path to check if a valid > /memory/reg node exists in the DTB file and if so, it > does not override it with the values in bi_memstart and > bi_memsize. This is particularly useful in multi-core > environments where the memory may be partitioned across > a large number of nodes. > > While the same can be accomplished on certain boards (p1022ds > and p1_p2_rdb) by using the bootm_low and bootm_size > environment variables, that solution is not universal and > requires adding code ft_board_setup() for any new board > that wants to support AMP operation. Also, given that the > DTB is already used to partition board devices (see commit > dc2e673 in the Linux kernel tree), it makes sense to > allow memory to be partitioned the same way from a user > configuration perspective. > > This patch allows for the user to override the DTB file parameters > on the p1022ds and p1_p2_rdb boards by setting the bootm_low and > bootm_size to something other than bi_memstart and bi_memsize. > In the long-term, those env variables should be depecrated and > removed and system implementors should provide the memory > partitioning information in the DTB. > > Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com> > Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com> I am not sure this is a good idea. So far we have a pretty clear definition of responsibilities. U-Boot does the low level initialization, including the sizing and testing of the system memory. U-Boot then passes its results to Linux in the (modified by U-Boot) device tree. Your patch inverts this situation, at least for the memory. I agree that there may be situations where you want an easy way to pass such information. But then let's handle this right. If you define that the device tree is the "master" for information about the memory layout (and potentially other hardware specifics), then you should be consequent and pass make U-Boot process this information. We've discussed before that there are a number of cases where it would be nice if U-Boot itself could be configured usign a device tree. This appears to be another one. What do you think? Best regards, Wolfgang Denk
On 12/06/2010 10:52 PM, Wolfgang Denk wrote: > Dear Deepak Saxena, > > I am not sure this is a good idea. > > So far we have a pretty clear definition of responsibilities. > U-Boot does the low level initialization, including the sizing and > testing of the system memory. U-Boot then passes its results to Linux > in the (modified by U-Boot) device tree. > > Your patch inverts this situation, at least for the memory. > > I agree that there may be situations where you want an easy way to > pass such information. But then let's handle this right. > > If you define that the device tree is the "master" for information > about the memory layout (and potentially other hardware specifics), > then you should be consequent and pass make U-Boot process this > information. We've discussed before that there are a number of cases > where it would be nice if U-Boot itself could be configured usign a > device tree. This appears to be another one. > > What do you think? Hi Wolfgang, Thanks for the response, I have a few different thoughts on the subject. I am a big fan of having consistent and clear definitions of responsibilities; however, I think the model of having U-Boot handle the creation of memory nodes in the DTB does not scale cleanly to users configuring, deploying, and managing complicated AMP environments. While we could provide a method to provide this information at build time to make U-Boot, this forces a static memory partitioning on the system and does not mesh well with use cases where developers may be testing different system layout options as it would require a rebuild and reflash every time a new configuration is to be tested. In certain environments, a developer may not be permitted to reflash the bootloader on a board shared by others (such as a remote HW lab). The bootm_low and bootm_size variables are an attempt to get around this by overriding what U-Boot knows about the system but I think those also don't scale well when we start dealing with large numbers of cores (32+) with independent OS instances on them for some of the same reasons. I think it is far simpler to have some custom scripts to spit out new DTB files that uBoot is configured to load every time it boots than to have to change a bunch of environment variables on boot. In the multi-node environments, we can't simply tell U-Boot to process the DTB to determine how much memory is in the system as there is one DTB per AMP node. One idea that comes to mind but that I think is not the right way to go due to complexity is to have the concept of nested DTBs that can define multiple operational "machines" and U-Boot knows how to read this and send the right sub-DTB to the right kernel image. I'm new to U-Boot development so would like to hear about the other use cases you mentioned (pointer to email threads are OK) so I can have a better understanding of the overall issues. Thinking about this at a higher level, I think the question is where is the delineation between board bringup/configuration and run time configuration management? Scanning memory and determining how much exists and programming the memory controller is a board-bring up and configuration task that a bootloader has traditionally done. Partitioning for AMP operation is about managing what and how is running on top of the bootloader. How much knowledge should the bootloader have about this? The approach of providing the memory partitioning in the DTB basically removes any of this knowledge from U-Boot, while the other approaches (bootm, build-time configuration) make U-Boot very aware and tied to what might be running above and reduce flexibility to easily change that. Thanks, ~Deepak
On 12/06/2010 10:52 PM, Wolfgang Denk wrote: > If you define that the device tree is the "master" for information > about the memory layout (and potentially other hardware specifics), > then you should be consequent and pass make U-Boot process this > information. We've discussed before that there are a number of cases > where it would be nice if U-Boot itself could be configured usign a > device tree. This appears to be another one. I *think* what you're suggesting is basically providing u-boot with a single device tree, even when it will load multiple operating systems. The tree would then look something like this: / cpus ... memory reg = <0 20000000> soc ... partitions partition@0 memory reg = <0 10000000> partition@1 memory reg = <10000000 10000000> U-boot would then be responsible for constructing multiple device trees (one for each partition) itself, based on the additional information found in the "partitions" node. Is that correct? Hollis Blanchard Mentor Graphics, Embedded Systems Division
Dear Deepak, In message <4CFE775C.6050001@mentor.com> you wrote: > > I am a big fan of having consistent and clear definitions of > responsibilities; however, I think the model of having U-Boot > handle the creation of memory nodes in the DTB does not scale > cleanly to users configuring, deploying, and managing complicated > AMP environments. I agree with you, but I think this is just one part (and eventually a minor part) of the issue. I would not protest if you say thatthe whol (static, compile time) configuration of U-Boot does not scale well for such requirements. So far we usually had pretty static board configurations, and a static compile time description was all we needed. Some developers consider even simple extensions like auto-sizing the available RAM as unnecessary luxury that just inreases the boot time and memory footprint. When it comes to more complicated setups we should provide means for a more dynamic configuration - this has been discussed before, and there was a general agreement that the device tree would be a usable way to implement such a description of the hardware. So what I'm proposing is not an opposite to what you have in mind, it just takes it one step further, and makes the whole system consistent again. Waht I don't like is the tunneling of information through U-Boot, while U-Boot actually needs and uses this very information as well. > While we could provide a method to provide this information at build > time to make U-Boot, this forces a static memory partitioning on the > system and does not mesh well with use cases where developers may This is NOT what I suggested. > The bootm_low and bootm_size variables are an attempt to get around > this by overriding what U-Boot knows about the system but I think > those also don't scale well when we start dealing with large numbers > of cores (32+) with independent OS instances on them for some of the > same reasons. I think it is far simpler to have some custom scripts to > spit out new DTB files that uBoot is configured to load every time it > boots than to have to change a bunch of environment variables on boot. Again, there is no conflict between your statement and what I suggested. > In the multi-node environments, we can't simply tell U-Boot to process > the DTB to determine how much memory is in the system as there is one > DTB per AMP node. One idea that comes to mind but that I think is not Please explain: you can use the DT to tell Linux (or other OS) how much memory they shoulduse, but you cannot use the same mechanism to pass the same information to U-Boot? > the right way to go due to complexity is to have the concept of > nested DTBs that can define multiple operational "machines" and > U-Boot knows how to read this and send the right sub-DTB to the > right kernel image. This is a technical details that can and should be discussed when we have an agreement about the basic mechanism. > I'm new to U-Boot development so would like to hear about the other use > cases you mentioned (pointer to email threads are OK) so I can have a > better understanding of the overall issues. There are many board vendors who shipt boards with different configurations - with or without NAND flash; with or without other peripherals like CAN contollers, LCD, etc.; with different LCD sizes and types, in portrait or landscape orientation, etc. Some of these features can be determined by probing the hardware, others (like the orientation of a LCD) cannot. It is sometimes a maintenance nightmare to provide tens of different configurations of U-Boot for a single product. Being able to cinfigure available hardware through the DT, and using a single common binary image of U-Boot for such systems would be a great benefit. > Thinking about this at a higher level, I think the question is where > is the delineation between board bringup/configuration and run time > configuration management? Scanning memory and determining how much > exists and programming the memory controller is a board-bring up and > configuration task that a bootloader has traditionally done. And probaly one it still has to do, as there are good chances that you don't know the actual memory size in advance - like on systems that come in several configruations but use a common U-Boot image, or systems where the user can plug in DIMMs of different size. > Partitioning for AMP operation is about managing what and how is running > on top of the bootloader. How much knowledge should the bootloader have > about this? The approach of providing the memory partitioning in the DTB > basically removes any of this knowledge from U-Boot, while the I see many use cases where this is simply not possible, because you need need the help of the bootloader to determine parameters that are not known in advance, and that thus cannot be encoded in the DT. Memory size if a very typical example for such a parameter. It may be OK for the use case you have currently in mind to use a fixed size, but this covers just a few systems and is not flexible enough for general use. > other approaches (bootm, build-time configuration) make U-Boot very > aware and tied to what might be running above and reduce flexibility > to easily change that. Again, this was NOT what I suggested. Best regards, Wolfgang Denk
Dear Hollis, In message <4CFE7FA8.2030701@mentor.com> you wrote: > On 12/06/2010 10:52 PM, Wolfgang Denk wrote: > > If you define that the device tree is the "master" for information > > about the memory layout (and potentially other hardware specifics), > > then you should be consequent and pass make U-Boot process this > > information. We've discussed before that there are a number of cases > > where it would be nice if U-Boot itself could be configured usign a > > device tree. This appears to be another one. > I *think* what you're suggesting is basically providing u-boot with a > single device tree, even when it will load multiple operating systems. > The tree would then look something like this: To be honest - I have not spent thoughts yet how this can be implemented. I would expect that there might be some common part, but there will eventually also be per-core configurations. > cpus > ... > memory > reg = <0 20000000> > soc > ... > partitions > partition@0 > memory > reg = <0 10000000> > partition@1 > memory > reg = <10000000 10000000> > > U-boot would then be responsible for constructing multiple device trees > (one for each partition) itself, based on the additional information > found in the "partitions" node. > > Is that correct? I did not think about that. I did not think about how to boot an OS and how to provide a DT to these. My concerns are still on a lower level. Memory initialization is a very basic task of the boot loader. We are discussiong to move the description of this resource out of U-Boot (where it was traditionally statically coinfigured at compile time, with the exception of auti-sizing). When doing so, we should not let U-Boot live in a different world, or let it operate on a different set of information. If the description of a resource is in the DT, then U-Boot has to use this information (from the DT) for all operations that deal with this resource. Just passing such information to the OS, behind U-Boot's back and with U-Boot having an independt (and probably different) view makes no sense to me. There are a number of features (persistent RAM, shared log buffer, shared video buffers, ...) where U-Boot and Linux need to have exactly the same understanding about available and usable memory. I just want to make sure that future extensions will allow to keep these features instead of breaking them. Best regards, Wolfgang Denk
On Mon, 6 Dec 2010 16:56:26 -0800 Deepak Saxena <deepak_saxena@mentor.com> wrote: > +/* > + * Check to see if an valid memory/reg property exists > + * in the fdt. If so, we do not overwrite it with what's > + * been scanned. > + * > + * Valid mean all the following: > + * > + * - Memory node has a device-type of "memory" > + * - A reg property exists which: > + * + has exactly as many cells as #address-cells + #size-cells > + * + provides a range that is within [bi_memstart, bi_memstart + > bi_memsize] > + */ This will get false positives -- a lot of existing device tree templates have something like this: memory { device_type = "memory"; reg = <0 0 0 0>; // Filled by U-Boot }; -Scott
On 12/07/2010 11:09 AM, Wolfgang Denk wrote: > There are many board vendors who shipt boards with different > configurations - with or without NAND flash; with or without other > peripherals like CAN contollers, LCD, etc.; with different LCD sizes > and types, in portrait or landscape orientation, etc. Some of these > features can be determined by probing the hardware, others (like the > orientation of a LCD) cannot. It is sometimes a maintenance nightmare > to provide tens of different configurations of U-Boot for a single > product. Being able to cinfigure available hardware through the DT, > and using a single common binary image of U-Boot for such systems > would be a great benefit. That's fine, but so far I don't see how it's related. This is information u-boot needs during its own initialization, right? We need a way for our tools to specify information to the kernels' initialization, and still want u-boot to do all the hardware configuration it does today. It really doesn't matter to us if in the future u-boot uses device trees for that configuration: we just need a way to interact with the kernels. Hollis Blanchard Mentor Graphics, Embedded Systems Division
On 12/07/2010 11:09 AM, Wolfgang Denk wrote: > So far we usually had pretty static board configurations, and a static > compile time description was all we needed. Some developers consider > even simple extensions like auto-sizing the available RAM as > unnecessary luxury that just inreases the boot time and memory > footprint. > > When it comes to more complicated setups we should provide means for a > more dynamic configuration - this has been discussed before, and there > was a general agreement that the device tree would be a usable way to > implement such a description of the hardware. > > So what I'm proposing is not an opposite to what you have in mind, it > just takes it one step further, and makes the whole system consistent > again. > > Waht I don't like is the tunneling of information through U-Boot, > while U-Boot actually needs and uses this very information as well. I won't argue with you on this front. Having U-Boot determine some board information from the DT and determine other board information at run-time is not consistent and confusing. >> While we could provide a method to provide this information at build >> time to make U-Boot, this forces a static memory partitioning on the >> system and does not mesh well with use cases where developers may > > This is NOT what I suggested. Thanks for clarifying that. You had stated "pass make U-Boot process this information" and I read that as in passing the information to the build ("make") process. >> In the multi-node environments, we can't simply tell U-Boot to process >> the DTB to determine how much memory is in the system as there is one >> DTB per AMP node. One idea that comes to mind but that I think is not > > Please explain: you can use the DT to tell Linux (or other OS) how > much memory they shoulduse, but you cannot use the same mechanism to > pass the same information to U-Boot? I'm not against U-Boot using this information, I'm just not sure how to do this without adding quite a bit of complexity to the code base. We would have to have U-Boot parse the memory nodes, validate them, check for overlapping regions, check for holes, etc. I'm not arguing that it is not doable, but wondering if adding this complexity is worth if the scanning of memory and passing that information to the kernel works for the majority of use cases. What I'm trying to do is support a special use case, so what about wrapping support for simply passing the memory nodes from the DT to the kernel around a CONFIG option (CONFIG_OF_MEMORY_PASSTHROUGH?) that can be enabled by system implementers who need this and are running on fairly controlled environments while the larger issue of how to use the DT is hashed out? >> the right way to go due to complexity is to have the concept of >> nested DTBs that can define multiple operational "machines" and >> U-Boot knows how to read this and send the right sub-DTB to the >> right kernel image. > > This is a technical details that can and should be discussed when we > have an agreement about the basic mechanism. > >> I'm new to U-Boot development so would like to hear about the other use >> cases you mentioned (pointer to email threads are OK) so I can have a >> better understanding of the overall issues. > > There are many board vendors who shipt boards with different > configurations - with or without NAND flash; with or without other > peripherals like CAN contollers, LCD, etc.; with different LCD sizes > and types, in portrait or landscape orientation, etc. Some of these > features can be determined by probing the hardware, others (like the > orientation of a LCD) cannot. It is sometimes a maintenance nightmare > to provide tens of different configurations of U-Boot for a single > product. Being able to cinfigure available hardware through the DT, > and using a single common binary image of U-Boot for such systems > would be a great benefit. I completely understand your perspective here and agree with it. DT has made the kernel easier to maintain across multiple similar platforms so it makes sense to leverage the same technology in U-Boot. >> Partitioning for AMP operation is about managing what and how is running >> on top of the bootloader. How much knowledge should the bootloader have >> about this? The approach of providing the memory partitioning in the DTB >> basically removes any of this knowledge from U-Boot, while the > > I see many use cases where this is simply not possible, because you > need need the help of the bootloader to determine parameters that are > not known in advance, and that thus cannot be encoded in the DT. > Memory size if a very typical example for such a parameter. It may be > OK for the use case you have currently in mind to use a fixed size, > but this covers just a few systems and is not flexible enough for > general use. Again, I understand this, though I am not 100% sure there is a way to do this in a completely generic way off the top of my head. There are use cases where we must rely on U-Boot to scan and determine memory size and there are use cases where a system is designed and not changeable and users need to be able to reconfigure system partitioning on the fly. Are you open to the CONFIG option as a first step towards supporting the later use case? Thanks, ~Deepak
On 12/07/2010 01:22 PM, Scott Wood wrote: > On Mon, 6 Dec 2010 16:56:26 -0800 > Deepak Saxena <deepak_saxena@mentor.com> wrote: > >> +/* >> + * Check to see if an valid memory/reg property exists >> + * in the fdt. If so, we do not overwrite it with what's >> + * been scanned. >> + * >> + * Valid mean all the following: >> + * >> + * - Memory node has a device-type of "memory" >> + * - A reg property exists which: >> + * + has exactly as many cells as #address-cells + #size-cells >> + * + provides a range that is within [bi_memstart, bi_memstart + >> bi_memsize] >> + */ > > This will get false positives -- a lot of existing device tree > templates have something like this: ACK. The code in the patch actually checks for this, just didn't point it out in the comments. ~deepak
On Wed, 8 Dec 2010 10:59:44 -0800 Deepak Saxena <deepak_saxena@mentor.com> wrote: > On 12/07/2010 01:22 PM, Scott Wood wrote: > > On Mon, 6 Dec 2010 16:56:26 -0800 > > Deepak Saxena <deepak_saxena@mentor.com> wrote: > > > >> +/* > >> + * Check to see if an valid memory/reg property exists > >> + * in the fdt. If so, we do not overwrite it with what's > >> + * been scanned. > >> + * > >> + * Valid mean all the following: > >> + * > >> + * - Memory node has a device-type of "memory" > >> + * - A reg property exists which: > >> + * + has exactly as many cells as #address-cells + #size-cells > >> + * + provides a range that is within [bi_memstart, bi_memstart + > >> bi_memsize] > >> + */ > > > > This will get false positives -- a lot of existing device tree > > templates have something like this: > > ACK. The code in the patch actually checks for this, just didn't point > it out in the comments. Ah, I looked for that and missed it. But there are also some boards that have socketed RAM, but for some reason have a real memory size in the device tree (corresponding to what the board ships with, probably). I think this is something that should have to be selectable by the board config (for image size reasons as well). Also some nits: > +static int ft_validate_memory(void *blob, bd_t *bd) > +{ > + int nodeoffset; > + u32 *addrcell = (u32*)fdt_getprop(blob, 0, "#address-cells", NULL); > + u32 *sizecell = (u32*)fdt_getprop(blob, 0, "#size-cells", NULL); const u32 *, and eliminate the cast. > + u64 reg_base, reg_size; > + void *reg, *dtype; > + int len; > + > + if ((nodeoffset = fdt_path_offset(blob, "/memory")) >= 0) > + { Brace on same line as if Don't put the assignment inside the if statement. > + dtype = fdt_getprop(blob, nodeoffset, "device_type", &len); > + if (!dtype || (strcmp(dtype, "memory") != 0)) > + return 0; > + > + reg = fdt_getprop(blob, nodeoffset, "reg", &len); > + if (reg && len == ((*addrcell + *sizecell) * 4)) { > + if (*addrcell == 2) { > + reg_base = ((u64*)reg)[0]; > + reg_size = ((u64*)reg)[1]; > + } else { > + reg_base = ((u32*)reg)[0]; > + reg_size = ((u32*)reg)[1]; > + } This only works on big-endian. > + if ((reg_size) && > + (reg_base >= (u64)bd->bi_memstart) && > + ((reg_size + reg_base) > + <= ((u64)bd->bi_memstart + (u64)bd->bi_memsize))) > + return 1; > + } Probably want to complain to the user if reg is invalid and not zero/missing. -Scott
On Dec 8, 2010, at 11:11 AM, Scott Wood wrote: > Probably want to complain to the user if reg is invalid and not > zero/missing. I think you guys are making this too complicated. There are many ways to pass stupid mistakes via a device tree, don't get carried away trying to single out this one for error checking where the user is likely to really know what they are doing. This isn't a required specification to get correct, without anything u-boot will provide the proper information. Thanks. -- Dan
On Wed, 8 Dec 2010 11:22:59 -0800 Dan Malek <ppc6dev@digitaldans.com> wrote: > > On Dec 8, 2010, at 11:11 AM, Scott Wood wrote: > > > Probably want to complain to the user if reg is invalid and not > > zero/missing. > > I think you guys are making this too complicated. > There are many ways to pass stupid mistakes via > a device tree, don't get carried away trying to single > out this one for error checking where the user is likely > to really know what they are doing. This isn't a required > specification to get correct, without anything u-boot > will provide the proper information. I don't think that's what this is for. I think it's for AMP scenarios where you want to carve up memory between guests -- in which case you want U-Boot to not overwrite the device tree with its own idea of how much memory is present. This patch was trying to guess whether that's the case based on whether the existing value looks reasonable, but I think it has to be an explicit configuration (board config, environment variable, etc). Once that is done, then the test should either go away, or complain -- don't just silently do something different if it's been told that the memory node is supposed to be complete and correct. As for the merits of error checking in general, obviously it's not going to be complete. But it can be useful to check for some common errors, such as when a board has multiple DTSes and the user has to pick the right one for how U-Boot has been configured. Maybe granting an AMP partition a memory region beyond the bounds of detected memory is another such case. -Scott
Dear Hollis Blanchard, In message <4CFFCEC1.6000103@mentor.com> you wrote: > On 12/07/2010 11:09 AM, Wolfgang Denk wrote: > > There are many board vendors who shipt boards with different > > configurations - with or without NAND flash; with or without other > > peripherals like CAN contollers, LCD, etc.; with different LCD sizes > > and types, in portrait or landscape orientation, etc. Some of these > > features can be determined by probing the hardware, others (like the > > orientation of a LCD) cannot. It is sometimes a maintenance nightmare > > to provide tens of different configurations of U-Boot for a single > > product. Being able to cinfigure available hardware through the DT, > > and using a single common binary image of U-Boot for such systems > > would be a great benefit. > That's fine, but so far I don't see how it's related. This is > information u-boot needs during its own initialization, right? Right. > We need a way for our tools to specify information to the kernels' > initialization, and still want u-boot to do all the hardware > configuration it does today. It really doesn't matter to us if in the > future u-boot uses device trees for that configuration: we just need a > way to interact with the kernels. When U-boot is supposed to do the hardware initialization, you probably include memory initialization, right? If so, should we then not make sure that U-Boot and the OS we're booting use the same information about this resource? If, on the other hand, you really want to make U-Boot ignore any /memory nodes in the device tree, then this should be done straight and without additional magic. In this case the DT should be responsible to provide all information the OS needs, and U-Boot should not touch this in any way. Then just do not call fdt_fixup_memory() at all for such configurations. I dislike the idea that U-Boot will not touch the DT entry in one situation, but will do so in another, without any visibility to (and eventually without awareness by) the user. If there is really a good reason for such magic, then this should be clearly documented (not only in some comment in some source file), and eventually fdt_fixup_memory() (or fdt_fixup_memory_banks() ?) should be made weak so it can be redefined by board-specific implementations. In any case, any such changes should be implemented in a generic way, i. e. not only for one specific processor family. Best regards, Wolfgang Denk
Dear Deepak Saxena, In message <4CFFD57C.1010601@mentor.com> you wrote: > > > Please explain: you can use the DT to tell Linux (or other OS) how > > much memory they shoulduse, but you cannot use the same mechanism to > > pass the same information to U-Boot? > > I'm not against U-Boot using this information, I'm just not sure how to > do this without adding quite a bit of complexity to the code base. We > would have to have U-Boot parse the memory nodes, validate them, check > for overlapping regions, check for holes, etc. I'm not arguing that it > is not doable, but wondering if adding this complexity is worth if the > scanning of memory and passing that information to the kernel works for > the majority of use cases. What I'm trying to do is support a special > use case, so what about wrapping support for simply passing the memory > nodes from the DT to the kernel around a CONFIG option > (CONFIG_OF_MEMORY_PASSTHROUGH?) that can be enabled by system > implementers who need this and are running on fairly controlled > environments while the larger issue of how to use the DT is hashed out? See my previous message to Hollis. If you really want U-Boot to keep it's fingers off the /memory nodes in the DT, then simply do that. But then please be consequent, add it for all architectures, and if enabled, then without magic where U-Boot will sometimes to this and other times do that. And provide documentation to the end user. Best regards, Wolfgang Denk
On 12/08/2010 12:53 PM, Wolfgang Denk wrote: > Dear Hollis Blanchard, > > In message<4CFFCEC1.6000103@mentor.com> you wrote: >> On 12/07/2010 11:09 AM, Wolfgang Denk wrote: >>> There are many board vendors who shipt boards with different >>> configurations - with or without NAND flash; with or without other >>> peripherals like CAN contollers, LCD, etc.; with different LCD sizes >>> and types, in portrait or landscape orientation, etc. Some of these >>> features can be determined by probing the hardware, others (like the >>> orientation of a LCD) cannot. It is sometimes a maintenance nightmare >>> to provide tens of different configurations of U-Boot for a single >>> product. Being able to cinfigure available hardware through the DT, >>> and using a single common binary image of U-Boot for such systems >>> would be a great benefit. >> That's fine, but so far I don't see how it's related. This is >> information u-boot needs during its own initialization, right? > Right. > >> We need a way for our tools to specify information to the kernels' >> initialization, and still want u-boot to do all the hardware >> configuration it does today. It really doesn't matter to us if in the >> future u-boot uses device trees for that configuration: we just need a >> way to interact with the kernels. > When U-boot is supposed to do the hardware initialization, you > probably include memory initialization, right? If so, should we then > not make sure that U-Boot and the OS we're booting use the same > information about this resource? Yes, I do include memory initialization. In our use case, u-boot must know about all memory (to configure the memory controller), but each OS is only made aware of a piece of it. They really do have different information about the resource. > If, on the other hand, you really want to make U-Boot ignore any > /memory nodes in the device tree, then this should be done straight > and without additional magic. In this case the DT should be > responsible to provide all information the OS needs, and U-Boot should > not touch this in any way. Then just do not call fdt_fixup_memory() > at all for such configurations. I think the current way that u-boot updates the memory node is valuable for other use cases. In particular, it is very convenient for single-OS systems. Our goal is to avoid affecting those use cases. > I dislike the idea that U-Boot will not touch the DT entry in one > situation, but will do so in another, without any visibility to (and > eventually without awareness by) the user. I see it as allowing the user to optionally override auto-detected information. The user has to go out of their way to request this behavior, so I think the visibility/awareness is there. The existing bootm_low/bootm_size facility does exactly this, but I think we can improve on its design. > If there is really a good reason for such magic, then this should be > clearly documented (not only in some comment in some source file), > and eventually fdt_fixup_memory() (or fdt_fixup_memory_banks() ?) > should be made weak so it can be redefined by board-specific > implementations. > > In any case, any such changes should be implemented in a generic way, > i. e. not only for one specific processor family. I agree that all (device tree aware) systems should behave consistently in this regard. Of course, in that case, we don't need weak functions? Documenting the behavior is a very good point. Hollis Blanchard Mentor Graphics, Embedded Systems Division
Dear Hollis, In message <4CFFF3C4.20800@mentor.com> you wrote: > > I think the current way that u-boot updates the memory node is valuable > for other use cases. In particular, it is very convenient for single-OS > systems. Our goal is to avoid affecting those use cases. > > I dislike the idea that U-Boot will not touch the DT entry in one > > situation, but will do so in another, without any visibility to (and > > eventually without awareness by) the user. > I see it as allowing the user to optionally override auto-detected > information. The user has to go out of their way to request this > behavior, so I think the visibility/awareness is there. OK - but then the user should really be requested to select the beviour. This should be done in an explicit and documented way, ant not based on some magic properties of the DT. > The existing bootm_low/bootm_size facility does exactly this, but I > think we can improve on its design. I have to admit that I also dislike the bootm_low / bootm_size stuff as it's really confusing to users, especially as the difference between bootm_size and CONFIG_SYS_BOOTMAPSZ (and BOOTMAPSZ, which is sometimes used instead) is not really clear. > I agree that all (device tree aware) systems should behave consistently > in this regard. Of course, in that case, we don't need weak functions? If you want to make this switchable at runtime, then we should probably use an environment setting. Best regards, Wolfgang Denk
Hi Wolfgang. On Dec 8, 2010, at 1:38 PM, Wolfgang Denk wrote: > If you want to make this switchable at runtime, then we should > probably use an environment setting. I experimented with this, but could never determine the best way to cover all behavior. Do we have a variable that indicates "don't update DT?" If so, it means we have to place all values in the DT, when it's really nice for U-Boot to do some of that for us. In fact, we want U-Boot to update many things it discovers, just not the few we wish to actually (sometimes) define for ourselves. The other alternative (granted, I'm not as smart as I used to be :-)) was to have an environment variable that specified which node we didn't want updated by U-Boot. For now, that seems reasonable since there is only one, but is that the general approach we want in the future? It also presents the challenge of having to update both environment and the provided DT. I just wanted to make these points, as I did try some alternatives but never found anything acceptable. By looking at key values in the DT, at least it was confined to one place. This feature is needed, so I propose we let it exist in the form we have found useful and let it evolve over time as others gain some experience. Thanks. -- Dan
Dear Dan, In message <0DDCBDA1-188F-433D-BDCC-5FDCF709A131@digitaldans.com> you wrote: > > > If you want to make this switchable at runtime, then we should > > probably use an environment setting. > > I experimented with this, but could never determine the > best way to cover all behavior. Do we have a variable that > indicates "don't update DT?" If so, it means we have to > place all values in the DT, when it's really nice for U-Boot > to do some of that for us. In fact, we want U-Boot to update > many things it discovers, just not the few we wish to actually > (sometimes) define for ourselves. "You can please all the people some of the time and some of the people all of the time but you can't please all the people all of the time." > The other alternative (granted, I'm not as smart as I used > to be :-)) was to have an environment variable that specified > which node we didn't want updated by U-Boot. For now, > that seems reasonable since there is only one, but is that > the general approach we want in the future? It also presents > the challenge of having to update both environment and > the provided DT. I guess we can argue that the normal situation is that U-Boot will know how to update the DT such as needed to boot the OS. So what we are dealing with is a small percentage of cases where we need special behaviour, and where it may be acceptable if the solution is only semi-perfect ;-) My current thinking is to introduce something like dt_skip=memory,mac-address including eventually "dt_skip=ALL". This should cover most of the current use cases. If someone gets fancy he can add wildcard support. And if we need even more flexibility, we can add some "dt_include" with higher priority, so one could do for example dt_skip=ALL dt_include=memory to have only the memory node updated. Best regards, Wolfgang Denk
On Wed, 2010-12-08 at 23:34 +0100, Wolfgang Denk wrote: > Dear Dan, > > In message <0DDCBDA1-188F-433D-BDCC-5FDCF709A131@digitaldans.com> you wrote: > > > > > If you want to make this switchable at runtime, then we should > > > probably use an environment setting. > > > > I experimented with this, but could never determine the > > best way to cover all behavior. Do we have a variable that > > indicates "don't update DT?" If so, it means we have to > > place all values in the DT, when it's really nice for U-Boot > > to do some of that for us. In fact, we want U-Boot to update > > many things it discovers, just not the few we wish to actually > > (sometimes) define for ourselves. > > "You can please all the people some of the time and some of the people > all of the time but you can't please all the people all of the time." > > > The other alternative (granted, I'm not as smart as I used > > to be :-)) was to have an environment variable that specified > > which node we didn't want updated by U-Boot. For now, > > that seems reasonable since there is only one, but is that > > the general approach we want in the future? It also presents > > the challenge of having to update both environment and > > the provided DT. > > I guess we can argue that the normal situation is that U-Boot will > know how to update the DT such as needed to boot the OS. So what we > are dealing with is a small percentage of cases where we need special > behaviour, and where it may be acceptable if the solution is only > semi-perfect ;-) > > My current thinking is to introduce something like > > dt_skip=memory,mac-address I haven't followed the this thread too closely, but I was curious if you could do manual tweaks by using the 'bootm' and 'fdt' sub-commands. This could allow more fine grained control of the boot process and let you manually modify the DTB before booting to Linux. Eg make the 'bootcmd' environment variable be something like: bootm start $loadaddr; \ bootm loados; \ bootm ramdisk; \ bootm fdt; \ fdt boardsetup; \ fdt set memory reg "<0 10000000>"; \ bootm prep; \ bootm go Some dual core Freescale board's do somewhat similar operations for AMP operation (see doc/README.p2020rdb), although I haven't personally tried AMP. Best, Peter
On Dec 8, 2010, at 2:34 PM, Wolfgang Denk wrote: > "You can please all the people some of the time and some of the people > all of the time but you can't please all the people all of the time." Yes, I'm sometimes pleased.... :-) > My current thinking is to introduce something like ..... Well, that is pretty cool. > dt_skip=memory,mac-address Do we have to write a parser now, or is there something that currently exists to help out? :-) Thanks. -- Dan
Dear Dan, In message <750641C9-DC97-4923-B337-05A2F1BC9676@digitaldans.com> you wrote: > > Yes, I'm sometimes pleased.... :-) Good :-) > > My current thinking is to introduce something like ..... > > Well, that is pretty cool. > > > dt_skip=memory,mac-address > > Do we have to write a parser now, or is there something > that currently exists to help out? :-) We use the same format already to implement the hwconfig feature. It just needs to be moved to common code. Best regards, Wolfgang Denk
On 12/08/2010 02:34 PM, Wolfgang Denk wrote: > > I guess we can argue that the normal situation is that U-Boot will > know how to update the DT such as needed to boot the OS. So what we > are dealing with is a small percentage of cases where we need special > behaviour, and where it may be acceptable if the solution is only > semi-perfect ;-) > > My current thinking is to introduce something like > > dt_skip=memory,mac-address > > including eventually "dt_skip=ALL". This should cover most of the > current use cases. > > If someone gets fancy he can add wildcard support. > > And if we need even more flexibility, we can add some "dt_include" > with higher priority, so one could do for example > > dt_skip=ALL > dt_include=memory I imagine this being rather ugly to implement and to keep the code clean and maintained. Who parses these variables? Does each and every piece of code in U-Boot that now touches a piece of the DT need to check for this variable? I could see something like this working if there was a central DT handler that read nodes and then called platform-specific over-ride function, i.e.: for_each_node_in_dt() { if (dt_include(node->type)) platform_of_dt_node_process(node, boot_stage); } where boot_stage tells us whether we are at early init, about to boot an OS image, or in some other step in the process. This would provide a consistent method of handling that variable. Without something like this, I think and environment variable is just going to create confusion for users and developers. ~Deepak
Dear Deepak Saxena, In message <4D026BB2.6020609@mentor.com> you wrote: > On 12/08/2010 02:34 PM, Wolfgang Denk wrote: > > > > > I guess we can argue that the normal situation is that U-Boot will > > know how to update the DT such as needed to boot the OS. So what we > > are dealing with is a small percentage of cases where we need special > > behaviour, and where it may be acceptable if the solution is only > > semi-perfect ;-) > > > > My current thinking is to introduce something like > > > > dt_skip=memory,mac-address > > > > including eventually "dt_skip=ALL". This should cover most of the > > current use cases. > > > > If someone gets fancy he can add wildcard support. > > > > And if we need even more flexibility, we can add some "dt_include" > > with higher priority, so one could do for example > > > > dt_skip=ALL > > dt_include=memory > > I imagine this being rather ugly to implement and to keep the code clean > and maintained. Who parses these variables? Does each and every piece of > code in U-Boot that now touches a piece of the DT need to check for this > variable? I could see something like this working > if there was a central DT handler that read nodes and then called > platform-specific over-ride function, i.e.: > > for_each_node_in_dt() { > if (dt_include(node->type)) > platform_of_dt_node_process(node, boot_stage); > } > > where boot_stage tells us whether we are at early init, about to > boot an OS image, or in some other step in the process. > > This would provide a consistent method of handling that variable. > Without something like this, I think and environment variable is just > going to create confusion for users and developers. You just described what the implementation should look like. Thanks. Wolfgang Denk
Dear Deepak Saxena, > commit 341764495180a712b9aaccfa0479b2ff7e44e35b > Author: Deepak Saxena <deepak_saxena@mentor.com> > Date: Mon Dec 6 15:52:07 2010 -0800 > > Honor /memory/reg node in DTB files > > This patch adds code to the bootm path to check if a valid > /memory/reg node exists in the DTB file and if so, it > does not override it with the values in bi_memstart and > bi_memsize. This is particularly useful in multi-core > environments where the memory may be partitioned across > a large number of nodes. > > While the same can be accomplished on certain boards (p1022ds > and p1_p2_rdb) by using the bootm_low and bootm_size > environment variables, that solution is not universal and > requires adding code ft_board_setup() for any new board > that wants to support AMP operation. Also, given that the > DTB is already used to partition board devices (see commit > dc2e673 in the Linux kernel tree), it makes sense to > allow memory to be partitioned the same way from a user > configuration perspective. > > This patch allows for the user to override the DTB file parameters > on the p1022ds and p1_p2_rdb boards by setting the bootm_low and > bootm_size to something other than bi_memstart and bi_memsize. > In the long-term, those env variables should be depecrated and > removed and system implementors should provide the memory > partitioning information in the DTB. > > Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com> > Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com> > > --- > > See http://lists.denx.de/pipermail/u-boot/2010-December/083057.html > for initial proposal on this. > > diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c > b/arch/powerpc/cpu/mpc85xx/fdt.c index 4540364..6d384e3 100644 > --- a/arch/powerpc/cpu/mpc85xx/fdt.c > +++ b/arch/powerpc/cpu/mpc85xx/fdt.c > @@ -377,6 +377,55 @@ static void ft_fixup_qe_snum(void *blob) > } > #endif > > +/* > + * Check to see if an valid memory/reg property exists > + * in the fdt. If so, we do not overwrite it with what's > + * been scanned. > + * > + * Valid mean all the following: > + * > + * - Memory node has a device-type of "memory" > + * - A reg property exists which: > + * + has exactly as many cells as #address-cells + #size-cells > + * + provides a range that is within [bi_memstart, bi_memstart + > bi_memsize] > + */ > +static int ft_validate_memory(void *blob, bd_t *bd) > +{ > + int nodeoffset; > + u32 *addrcell = (u32*)fdt_getprop(blob, 0, "#address-cells", NULL); > + u32 *sizecell = (u32*)fdt_getprop(blob, 0, "#size-cells", NULL); > + u64 reg_base, reg_size; > + void *reg, *dtype; > + int len; > + > + if ((nodeoffset = fdt_path_offset(blob, "/memory")) >= 0) > + { > + dtype = fdt_getprop(blob, nodeoffset, "device_type", &len); > + if (!dtype || (strcmp(dtype, "memory") != 0)) > + return 0; > + > + reg = fdt_getprop(blob, nodeoffset, "reg", &len); > + if (reg && len == ((*addrcell + *sizecell) * 4)) { > + if (*addrcell == 2) { > + reg_base = ((u64*)reg)[0]; > + reg_size = ((u64*)reg)[1]; > + } else { > + reg_base = ((u32*)reg)[0]; > + reg_size = ((u32*)reg)[1]; > + } > + > + if ((reg_size) && > + (reg_base >= (u64)bd->bi_memstart) && > + ((reg_size + reg_base) > + <= ((u64)bd->bi_memstart + (u64)bd->bi_memsize))) > + return 1; > + } > + } > + > + return 0; > + > +} > + > void ft_cpu_setup(void *blob, bd_t *bd) > { > int off; > @@ -434,7 +483,8 @@ void ft_cpu_setup(void *blob, bd_t *bd) > "clock-frequency", CONFIG_SYS_CLK_FREQ, 1); > #endif > > - fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize); > + if (!ft_validate_memory(blob, bd)) > + fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd- >bi_memsize); > > #ifdef CONFIG_MP > ft_fixup_cpu(blob, (u64)bd->bi_memstart + (u64)bd->bi_memsize); > diff --git a/board/freescale/p1022ds/p1022ds.c > b/board/freescale/p1022ds/p1022ds.c > index 5cdee9f..7378d88 100644 > --- a/board/freescale/p1022ds/p1022ds.c > +++ b/board/freescale/p1022ds/p1022ds.c > @@ -320,7 +320,8 @@ void ft_board_setup(void *blob, bd_t *bd) > base = getenv_bootm_low(); > size = getenv_bootm_size(); > > - fdt_fixup_memory(blob, (u64)base, (u64)size); > + if (base != (phys_addr_t)bd->bi_memstart && size != > (phys_addr_t)bd->bi_memsize) > + fdt_fixup_memory(blob, (u64)base, (u64)size); > > FT_FSL_PCI_SETUP; > > diff --git a/board/freescale/p1_p2_rdb/p1_p2_rdb.c > b/board/freescale/p1_p2_rdb/p1_p2_rdb.c > index fae31f2..5e4adc6 100644 > --- a/board/freescale/p1_p2_rdb/p1_p2_rdb.c > +++ b/board/freescale/p1_p2_rdb/p1_p2_rdb.c > @@ -220,9 +220,10 @@ void ft_board_setup(void *blob, bd_t *bd) > base = getenv_bootm_low(); > size = getenv_bootm_size(); > > - ft_pci_board_setup(blob); > + if (base != (phys_addr_t)bd->bi_memstart && size != > (phys_addr_t)bd->bi_memsize) > + fdt_fixup_memory(blob, (u64)base, (u64)size); > > - fdt_fixup_memory(blob, (u64)base, (u64)size); > + ft_pci_board_setup(blob); > } > #endif > What was the conclusion on this patch ?
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 4540364..6d384e3 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -377,6 +377,55 @@ static void ft_fixup_qe_snum(void *blob) } #endif +/* + * Check to see if an valid memory/reg property exists + * in the fdt. If so, we do not overwrite it with what's + * been scanned. + * + * Valid mean all the following: + * + * - Memory node has a device-type of "memory" + * - A reg property exists which: + * + has exactly as many cells as #address-cells + #size-cells + * + provides a range that is within [bi_memstart, bi_memstart + bi_memsize] + */ +static int ft_validate_memory(void *blob, bd_t *bd) +{ + int nodeoffset; + u32 *addrcell = (u32*)fdt_getprop(blob, 0, "#address-cells", NULL); + u32 *sizecell = (u32*)fdt_getprop(blob, 0, "#size-cells", NULL); + u64 reg_base, reg_size; + void *reg, *dtype; + int len; + + if ((nodeoffset = fdt_path_offset(blob, "/memory")) >= 0) + { + dtype = fdt_getprop(blob, nodeoffset, "device_type", &len); + if (!dtype || (strcmp(dtype, "memory") != 0)) + return 0; + + reg = fdt_getprop(blob, nodeoffset, "reg", &len); + if (reg && len == ((*addrcell + *sizecell) * 4)) { + if (*addrcell == 2) { + reg_base = ((u64*)reg)[0]; + reg_size = ((u64*)reg)[1]; + } else { + reg_base = ((u32*)reg)[0]; + reg_size = ((u32*)reg)[1]; + } + + if ((reg_size) && + (reg_base >= (u64)bd->bi_memstart) && + ((reg_size + reg_base) + <= ((u64)bd->bi_memstart + (u64)bd->bi_memsize))) + return 1; + } + } + + return 0; + +} + void ft_cpu_setup(void *blob, bd_t *bd) { int off; @@ -434,7 +483,8 @@ void ft_cpu_setup(void *blob, bd_t *bd) "clock-frequency", CONFIG_SYS_CLK_FREQ, 1); #endif - fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize); + if (!ft_validate_memory(blob, bd)) + fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize); #ifdef CONFIG_MP ft_fixup_cpu(blob, (u64)bd->bi_memstart + (u64)bd->bi_memsize); diff --git a/board/freescale/p1022ds/p1022ds.c b/board/freescale/p1022ds/p1022ds.c index 5cdee9f..7378d88 100644 --- a/board/freescale/p1022ds/p1022ds.c +++ b/board/freescale/p1022ds/p1022ds.c @@ -320,7 +320,8 @@ void ft_board_setup(void *blob, bd_t *bd) base = getenv_bootm_low(); size = getenv_bootm_size(); - fdt_fixup_memory(blob, (u64)base, (u64)size); + if (base != (phys_addr_t)bd->bi_memstart && size != (phys_addr_t)bd->bi_memsize) + fdt_fixup_memory(blob, (u64)base, (u64)size); FT_FSL_PCI_SETUP; diff --git a/board/freescale/p1_p2_rdb/p1_p2_rdb.c b/board/freescale/p1_p2_rdb/p1_p2_rdb.c index fae31f2..5e4adc6 100644 --- a/board/freescale/p1_p2_rdb/p1_p2_rdb.c +++ b/board/freescale/p1_p2_rdb/p1_p2_rdb.c @@ -220,9 +220,10 @@ void ft_board_setup(void *blob, bd_t *bd) base = getenv_bootm_low(); size = getenv_bootm_size(); - ft_pci_board_setup(blob); + if (base != (phys_addr_t)bd->bi_memstart && size != (phys_addr_t)bd->bi_memsize) + fdt_fixup_memory(blob, (u64)base, (u64)size); - fdt_fixup_memory(blob, (u64)base, (u64)size); + ft_pci_board_setup(blob); } #endif
commit 341764495180a712b9aaccfa0479b2ff7e44e35b Author: Deepak Saxena <deepak_saxena@mentor.com> Date: Mon Dec 6 15:52:07 2010 -0800 Honor /memory/reg node in DTB files This patch adds code to the bootm path to check if a valid /memory/reg node exists in the DTB file and if so, it does not override it with the values in bi_memstart and bi_memsize. This is particularly useful in multi-core environments where the memory may be partitioned across a large number of nodes. While the same can be accomplished on certain boards (p1022ds and p1_p2_rdb) by using the bootm_low and bootm_size environment variables, that solution is not universal and requires adding code ft_board_setup() for any new board that wants to support AMP operation. Also, given that the DTB is already used to partition board devices (see commit dc2e673 in the Linux kernel tree), it makes sense to allow memory to be partitioned the same way from a user configuration perspective. This patch allows for the user to override the DTB file parameters on the p1022ds and p1_p2_rdb boards by setting the bootm_low and bootm_size to something other than bi_memstart and bi_memsize. In the long-term, those env variables should be depecrated and removed and system implementors should provide the memory partitioning information in the DTB. Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com> Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com> --- See http://lists.denx.de/pipermail/u-boot/2010-December/083057.html for initial proposal on this.