| Message ID | 1396504871-1454-7-git-send-email-tharvey@gateworks.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
Hi Tim, On 04/03/2014 09:01 AM, Tim Harvey wrote: > This can be used to pass info between the SPL and u-boot. > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > arch/arm/include/asm/arch-mx6/sys_proto.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm/include/asm/arch-mx6/sys_proto.h b/arch/arm/include/asm/arch-mx6/sys_proto.h > index 38851a1..f43e09c 100644 > --- a/arch/arm/include/asm/arch-mx6/sys_proto.h > +++ b/arch/arm/include/asm/arch-mx6/sys_proto.h > @@ -39,4 +39,9 @@ int mxs_wait_mask_set(struct mxs_register_32 *reg, > int mxs_wait_mask_clr(struct mxs_register_32 *reg, > uint32_t mask, > unsigned int timeout); > + > +struct mx6_spl_data { > + uint8_t boot_mode_idx; > + uint32_t mem_dram_size; > +}; > #endif > While I'm in favor of having some way for SPL to pass data to U-Boot, I don't think this patch achieves this as long as we don't have a common mechanism that makes use of this struct. At the very least I would've expected to see a #define that is shared by SPL and U-Boot that defines the address for this struct (if we were to use the ventana implementation).
Hi Tim, On 03/04/2014 08:01, Tim Harvey wrote: > This can be used to pass info between the SPL and u-boot. > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > arch/arm/include/asm/arch-mx6/sys_proto.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm/include/asm/arch-mx6/sys_proto.h b/arch/arm/include/asm/arch-mx6/sys_proto.h > index 38851a1..f43e09c 100644 > --- a/arch/arm/include/asm/arch-mx6/sys_proto.h > +++ b/arch/arm/include/asm/arch-mx6/sys_proto.h > @@ -39,4 +39,9 @@ int mxs_wait_mask_set(struct mxs_register_32 *reg, > int mxs_wait_mask_clr(struct mxs_register_32 *reg, > uint32_t mask, > unsigned int timeout); > + > +struct mx6_spl_data { > + uint8_t boot_mode_idx; > + uint32_t mem_dram_size; > +}; > #endif I see checking your patch that the MXS uses the same concept. And as far as I can see, boot_mode_idx is used only to print the boot devioce However, we have not generally a concept to pass data between SPL and u-boot. My question is even if it is really needed. The size of DRAM is retrived at run time by u-boot running get_ram_size(), that is a better solution. SPL is thought to generally load an image (of course, in most cases it is u-boot). In Falcon mode, the kernel is started without running u-boot, making this structure useless. Do we really need such a way (but then, it must be generalized as SPL API), or can we get rid of it ? Best regards, Stefano Babic
On Mon, Apr 14, 2014 at 5:35 AM, Stefano Babic <sbabic@denx.de> wrote: > Hi Tim, > > I see checking your patch that the MXS uses the same concept. And as far > as I can see, boot_mode_idx is used only to print the boot devioce Stefano, yes, that is where I got the concept from. > > However, we have not generally a concept to pass data between SPL and > u-boot. My question is even if it is really needed. The size of DRAM is > retrived at run time by u-boot running get_ram_size(), that is a better > solution. I've been told this before, but I've found that get_ram_size() will hang on an i.MX6 if you pass it a maxsize larger than the memory in your system. Perhaps you can verify you see the same behavior? > > SPL is thought to generally load an image (of course, in most cases it > is u-boot). In Falcon mode, the kernel is started without running > u-boot, making this structure useless. Do we really need such a way (but > then, it must be generalized as SPL API), or can we get rid of it ? As we have an EEPROM on the board that tells us the physical ram size, I use that to avoid the lockup. Eventually I would like to read and validate the entire EEPROM once in SPL and pass this to u-boot.img to avoid reading and validating it again. I think this is a good example of why sharing data between SPL and u-boot.img could be useful. Regards, Tim
Hi Tim, On 17/04/2014 08:07, Tim Harvey wrote: > On Mon, Apr 14, 2014 at 5:35 AM, Stefano Babic <sbabic@denx.de> wrote: >> Hi Tim, >> >> I see checking your patch that the MXS uses the same concept. And as far >> as I can see, boot_mode_idx is used only to print the boot devioce > > Stefano, > > yes, that is where I got the concept from. Well, it slipped into mainline, but adding such a structure only to have a better print is not very useful... > I've been told this before, but I've found that get_ram_size() will > hang on an i.MX6 if you pass it a maxsize larger than the memory in > your system. Perhaps you can verify you see the same behavior? get_ram_size() works if the memory is accessible, that means that no exception is generated when it tries to access. If the RAM controller is programmed for a larger amount of memory, because you can have board with different capacity of RAM, get_ram_size() finds dynamically the soldered value. However, if the RAM controller is programmed for an amount of RAM smaller as what get_ram_size() tries to identify, an exception is raised. In get_ram_size(start, size) we pass the maximum amount of memory that the function is allowed to check. I can imagine we are running in this case. Checking on i.MX6, we have both boards checking the size with get_ram_size() as mx6sabresd, as well as boards fixing at compile time the value of the RAM (nitrogen6x). >> SPL is thought to generally load an image (of course, in most cases it >> is u-boot). In Falcon mode, the kernel is started without running >> u-boot, making this structure useless. Do we really need such a way (but >> then, it must be generalized as SPL API), or can we get rid of it ? > > As we have an EEPROM on the board that tells us the physical ram size, > I use that to avoid the lockup. It seems weird. There is several boards in mainline (I am sure about Freescale's PowerPC) discovering the mounted RAM. Of course, this is simpler if many parameters (row/columns/...) are the same, I am not sure about this. Are we sure it is not possible here ? What does it happen with an inconsistent value in EEprom ? > Eventually I would like to read and > validate the entire EEPROM once in SPL and pass this to u-boot.img to > avoid reading and validating it again. I think this is a good example > of why sharing data between SPL and u-boot.img could be useful. I am not sure, and I doubt it is a good idea to use persistent data to store that. It is potentially dangerous, and if some reasons the EEprom is changed, the board could not boot at all. On the other side, I like the current concept that the SPL is simply a loader and can load images of different types, and fixing an API between SPL and U-Boot goes against this concept. Best regards, Stefano Babic
Hi Stefano, Tim, On 04/17/14 12:30, Stefano Babic wrote: > Hi Tim, > > On 17/04/2014 08:07, Tim Harvey wrote: >> On Mon, Apr 14, 2014 at 5:35 AM, Stefano Babic <sbabic@denx.de> wrote: >>> Hi Tim, >>> >>> I see checking your patch that the MXS uses the same concept. And as far >>> as I can see, boot_mode_idx is used only to print the boot devioce >> >> Stefano, >> >> yes, that is where I got the concept from. > > Well, it slipped into mainline, but adding such a structure only to have > a better print is not very useful... > >> I've been told this before, but I've found that get_ram_size() will >> hang on an i.MX6 if you pass it a maxsize larger than the memory in >> your system. Perhaps you can verify you see the same behavior? > > get_ram_size() works if the memory is accessible, that means that no > exception is generated when it tries to access. If the RAM controller is > programmed for a larger amount of memory, because you can have board > with different capacity of RAM, get_ram_size() finds dynamically the > soldered value. > > However, if the RAM controller is programmed for an amount of RAM > smaller as what get_ram_size() tries to identify, an exception is > raised. In get_ram_size(start, size) we pass the maximum amount of > memory that the function is allowed to check. I can imagine we are > running in this case. Checking on i.MX6, we have both boards checking > the size with get_ram_size() as mx6sabresd, as well as boards fixing at > compile time the value of the RAM (nitrogen6x). get_ram_size() works on cm-fx6 all DRAM configurations. > >>> SPL is thought to generally load an image (of course, in most cases it >>> is u-boot). In Falcon mode, the kernel is started without running >>> u-boot, making this structure useless. Do we really need such a way (but >>> then, it must be generalized as SPL API), or can we get rid of it ? >> >> As we have an EEPROM on the board that tells us the physical ram size, >> I use that to avoid the lockup. > > It seems weird. There is several boards in mainline (I am sure about > Freescale's PowerPC) discovering the mounted RAM. Of course, this is > simpler if many parameters (row/columns/...) are the same, I am not sure > about this. Even if the parameters are different, this should not result in lockup... AFAICS, lockup of that type has nothing to do with the DRAM itself, but the controller configuration. > Are we sure it is not possible here ? What does it happen > with an inconsistent value in EEprom ? > >> Eventually I would like to read and >> validate the entire EEPROM once in SPL and pass this to u-boot.img to >> avoid reading and validating it again. I think this is a good example >> of why sharing data between SPL and u-boot.img could be useful. > > I am not sure, and I doubt it is a good idea to use persistent data to > store that. It is potentially dangerous, and if some reasons the EEprom > is changed, the board could not boot at all. This is more a question of design and definition of what purpose the eeprom should serve, so I would let the vendor decide if he wants to depend on eeprom or not. > > On the other side, I like the current concept that the SPL is simply a > loader and can load images of different types, and fixing an API between > SPL and U-Boot goes against this concept. I agree on this one, there is no real need (at least currently) to introduce such an API. Also, there are cases (e.g. cm-fx6) where the DRAM size can be determined by just reading the MMDC configuration and thus spare additional get_ram_size() call from U-Boot.
Hi Igor, hi Tim On 17/04/2014 13:22, Igor Grinberg wrote: > > get_ram_size() works on cm-fx6 all DRAM configurations. As on most boards in mainline ;-) > >> >>>> SPL is thought to generally load an image (of course, in most cases it >>>> is u-boot). In Falcon mode, the kernel is started without running >>>> u-boot, making this structure useless. Do we really need such a way (but >>>> then, it must be generalized as SPL API), or can we get rid of it ? >>> >>> As we have an EEPROM on the board that tells us the physical ram size, >>> I use that to avoid the lockup. >> >> It seems weird. There is several boards in mainline (I am sure about >> Freescale's PowerPC) discovering the mounted RAM. Of course, this is >> simpler if many parameters (row/columns/...) are the same, I am not sure >> about this. > > Even if the parameters are different, this should not result in lockup... > AFAICS, lockup of that type has nothing to do with the DRAM itself, but > the controller configuration. Exactly, this is what I meant. Thanks for clarifying it better. > >> Are we sure it is not possible here ? What does it happen >> with an inconsistent value in EEprom ? >> >>> Eventually I would like to read and >>> validate the entire EEPROM once in SPL and pass this to u-boot.img to >>> avoid reading and validating it again. I think this is a good example >>> of why sharing data between SPL and u-boot.img could be useful. >> >> I am not sure, and I doubt it is a good idea to use persistent data to >> store that. It is potentially dangerous, and if some reasons the EEprom >> is changed, the board could not boot at all. > > This is more a question of design and definition of what purpose the eeprom > should serve, so I would let the vendor decide if he wants to depend on eeprom > or not. Agree. Parsing and evaluating vendor specific information can be done inside board specific part, as now in read_eeprom inside gw_ventana.c. Anyway, if there is a way to detect at runtime the hardware configuration, it remains a better way as to store the size into the Eeprom. Best regards, Stefano
On Thu, Apr 17, 2014 at 4:44 AM, Stefano Babic <sbabic@denx.de> wrote: > Hi Igor, hi Tim > > On 17/04/2014 13:22, Igor Grinberg wrote: > >> >> get_ram_size() works on cm-fx6 all DRAM configurations. > > As on most boards in mainline ;-) > It looks like I mis-interpreted the failure. This issue is that get_ram_size() only works with powers of 2 and maxes out at 2GB due to 32bit addressing. I was simply passing in a bogus value. I'm not sure how you would deal with detecting systems with 4GB but I don't need to support that for my boards currently. >> >>> >>>>> SPL is thought to generally load an image (of course, in most cases it >>>>> is u-boot). In Falcon mode, the kernel is started without running >>>>> u-boot, making this structure useless. Do we really need such a way (but >>>>> then, it must be generalized as SPL API), or can we get rid of it ? >>>> >>>> As we have an EEPROM on the board that tells us the physical ram size, >>>> I use that to avoid the lockup. >>> >>> It seems weird. There is several boards in mainline (I am sure about >>> Freescale's PowerPC) discovering the mounted RAM. Of course, this is >>> simpler if many parameters (row/columns/...) are the same, I am not sure >>> about this. >> >> Even if the parameters are different, this should not result in lockup... >> AFAICS, lockup of that type has nothing to do with the DRAM itself, but >> the controller configuration. > > Exactly, this is what I meant. Thanks for clarifying it better. > >> >>> Are we sure it is not possible here ? What does it happen >>> with an inconsistent value in EEprom ? >>> >>>> Eventually I would like to read and >>>> validate the entire EEPROM once in SPL and pass this to u-boot.img to >>>> avoid reading and validating it again. I think this is a good example >>>> of why sharing data between SPL and u-boot.img could be useful. >>> >>> I am not sure, and I doubt it is a good idea to use persistent data to >>> store that. It is potentially dangerous, and if some reasons the EEprom >>> is changed, the board could not boot at all. >> >> This is more a question of design and definition of what purpose the eeprom >> should serve, so I would let the vendor decide if he wants to depend on eeprom >> or not. > > Agree. Parsing and evaluating vendor specific information can be done > inside board specific part, as now in read_eeprom inside gw_ventana.c. > > Anyway, if there is a way to detect at runtime the hardware > configuration, it remains a better way as to store the size into the Eeprom. I agree and I definitely see the merits of completely de-coupling the SPL from u-boot.img so I will resort to reading the EEPROM in both places. Regards, Tim
On 04/18/14 09:35, Tim Harvey wrote: > On Thu, Apr 17, 2014 at 4:44 AM, Stefano Babic <sbabic@denx.de> wrote: >> Hi Igor, hi Tim >> >> On 17/04/2014 13:22, Igor Grinberg wrote: >> >>> >>> get_ram_size() works on cm-fx6 all DRAM configurations. >> >> As on most boards in mainline ;-) >> > > It looks like I mis-interpreted the failure. This issue is that > get_ram_size() only works with powers of 2 and maxes out at 2GB due to > 32bit addressing. I was simply passing in a bogus value. I'm not sure > how you would deal with detecting systems with 4GB but I don't need to > support that for my boards currently. > cm-fx6 supports 4GB and detects it properly.
On Sun, Apr 20, 2014 at 12:52 AM, Igor Grinberg <grinberg@compulab.co.il> wrote: > On 04/18/14 09:35, Tim Harvey wrote: >> On Thu, Apr 17, 2014 at 4:44 AM, Stefano Babic <sbabic@denx.de> wrote: >>> Hi Igor, hi Tim >>> >>> On 17/04/2014 13:22, Igor Grinberg wrote: >>> >>>> >>>> get_ram_size() works on cm-fx6 all DRAM configurations. >>> >>> As on most boards in mainline ;-) >>> >> >> It looks like I mis-interpreted the failure. This issue is that >> get_ram_size() only works with powers of 2 and maxes out at 2GB due to >> 32bit addressing. I was simply passing in a bogus value. I'm not sure >> how you would deal with detecting systems with 4GB but I don't need to >> support that for my boards currently. >> > > cm-fx6 supports 4GB and detects it properly. Igor, That's interesting. What value are you passing get_ram_size() then? Where is your code? I don't see any cm-fx6 in mainline or posted patchsets. Tim
Hi Tim, On 04/21/14 21:28, Tim Harvey wrote: > On Sun, Apr 20, 2014 at 12:52 AM, Igor Grinberg <grinberg@compulab.co.il> wrote: >> On 04/18/14 09:35, Tim Harvey wrote: >>> On Thu, Apr 17, 2014 at 4:44 AM, Stefano Babic <sbabic@denx.de> wrote: >>>> Hi Igor, hi Tim >>>> >>>> On 17/04/2014 13:22, Igor Grinberg wrote: >>>> >>>>> >>>>> get_ram_size() works on cm-fx6 all DRAM configurations. >>>> >>>> As on most boards in mainline ;-) >>>> >>> >>> It looks like I mis-interpreted the failure. This issue is that >>> get_ram_size() only works with powers of 2 and maxes out at 2GB due to >>> 32bit addressing. I was simply passing in a bogus value. I'm not sure >>> how you would deal with detecting systems with 4GB but I don't need to >>> support that for my boards currently. >>> >> >> cm-fx6 supports 4GB and detects it properly. > > Igor, > > That's interesting. What value are you passing get_ram_size() then? Well, we do it in several get_ram_size() calls and decide according to returned results. > Where is your code? I don't see any cm-fx6 in mainline or posted > patchsets. Right, it is work in progress. Currently, it is based on 2014.01. Once it is ready, we re-base and post it.
diff --git a/arch/arm/include/asm/arch-mx6/sys_proto.h b/arch/arm/include/asm/arch-mx6/sys_proto.h index 38851a1..f43e09c 100644 --- a/arch/arm/include/asm/arch-mx6/sys_proto.h +++ b/arch/arm/include/asm/arch-mx6/sys_proto.h @@ -39,4 +39,9 @@ int mxs_wait_mask_set(struct mxs_register_32 *reg, int mxs_wait_mask_clr(struct mxs_register_32 *reg, uint32_t mask, unsigned int timeout); + +struct mx6_spl_data { + uint8_t boot_mode_idx; + uint32_t mem_dram_size; +}; #endif
This can be used to pass info between the SPL and u-boot. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- arch/arm/include/asm/arch-mx6/sys_proto.h | 5 +++++ 1 file changed, 5 insertions(+)