Message ID | 574DA679.4090208@alse-fr.com |
---|---|
State | Changes Requested |
Delegated to: | Marek Vasut |
Headers | show |
On 05/31/2016 04:58 PM, Sylvain Lesne wrote: > Hi, Hi, > On 05/30/2016 05:22 PM, Marek Vasut wrote: >> Enable both features to reduce the SPL size by 6 kiB. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Chin Liang See <clsee@altera.com> >> Cc: Dinh Nguyen <dinguyen@opensource.altera.com> >> Cc: Pavel Machek <pavel@denx.de> >> Cc: Stefan Roese <sr@denx.de> > > I tried to use the raw MMC boot (on the sockit, with current master > + your patches), so I changed the following: > > --- a/include/configs/socfpga_common.h > +++ b/include/configs/socfpga_common.h > @@ -349,9 +349,10 @@ unsigned int cm_get_qspi_controller_clk_hz(void); > #define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot-dtb.img" > #define CONFIG_SPL_LIBDISK_SUPPORT > #else > -#define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION 3 > -#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0xa00 /* offset 2560 > sect (1M+256k) */ > -#define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS 800 /* 400 KB */ > +#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION 3 > +#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0x200 /* offset 512 > sect (256k) */ > +#define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS 1000 /* 512 KB */ This should be 0x800 (was probably a typo in the original). Otherwise this should not break anything, yeah. > +#define CONFIG_SPL_LIBDISK_SUPPORT > #endif > #endif > > AFAIK, there were two mistakes: > 1) FS_BOOT_PARTITION instead of RAW_MODE_U_BOOT > 2) CONFIG_SPL_LIBDISK_SUPPORT was missing In fact, LIBDISK support was not used before you added the CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION into the board config. The libdisk functions are needed for determining the start of the partition. Looking through the SPL MMC, the code originally loaded the u-boot image from a fixed offset on the card (2560 sectors), which is indeed not optimal. So yes, please send the above patch, it'd be a nice improvement. > (I edited the offset to be able to use u-boot-with-spl.sfp directly) > > With these settings, I think we run into the size problem reported > previously in the ML. > > So, enabling tiny printf could help, but we now get: > > disk/built-in.o: In function `part_get_info_extended': > ...u-boot/disk/part_dos.c:236: undefined reference to `sprintf' > > So I think we can just put "info->name[0] = 0;" instead of > the snprintf() calls, when using tiny printf. There is more of that stuff in disk/ , at least part_efi.c and part_iso.c suffer from the exact same problem. I am not sure if setting the name to '\0' wouldn't break anything, CCing Simon as he was recently digging in those areas. Also, take a look at this patch: https://patchwork.ozlabs.org/patch/626760/ > I can submit those changes as two patches if it makes sense. Please do. > Thanks, > Sylvain >
On 05/31/2016 05:42 PM, Marek Vasut wrote: > On 05/31/2016 04:58 PM, Sylvain Lesne wrote: >> Hi, > > Hi, > >> On 05/30/2016 05:22 PM, Marek Vasut wrote: >>> Enable both features to reduce the SPL size by 6 kiB. >>> >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Cc: Chin Liang See <clsee@altera.com> >>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com> >>> Cc: Pavel Machek <pavel@denx.de> >>> Cc: Stefan Roese <sr@denx.de> >> >> I tried to use the raw MMC boot (on the sockit, with current master >> + your patches), so I changed the following: >> >> --- a/include/configs/socfpga_common.h >> +++ b/include/configs/socfpga_common.h >> @@ -349,9 +349,10 @@ unsigned int cm_get_qspi_controller_clk_hz(void); >> #define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot-dtb.img" >> #define CONFIG_SPL_LIBDISK_SUPPORT >> #else >> -#define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION 3 >> -#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0xa00 /* offset 2560 >> sect (1M+256k) */ >> -#define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS 800 /* 400 KB */ >> +#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION 3 >> +#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0x200 /* offset 512 >> sect (256k) */ >> +#define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS 1000 /* 512 KB */ > > This should be 0x800 (was probably a typo in the original). > Otherwise this should not break anything, yeah. > I might be missing something obvious, but it actually seems that this define isn't used outside of the include/configs/ folder... >> +#define CONFIG_SPL_LIBDISK_SUPPORT >> #endif >> #endif >> >> AFAIK, there were two mistakes: >> 1) FS_BOOT_PARTITION instead of RAW_MODE_U_BOOT >> 2) CONFIG_SPL_LIBDISK_SUPPORT was missing > > In fact, LIBDISK support was not used before you added the > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION into the board config. > The libdisk functions are needed for determining the start of the > partition. Looking through the SPL MMC, the code originally loaded the > u-boot image from a fixed offset on the card (2560 sectors), which is > indeed not optimal. > I agree, but I thought that the CONFIG_SYS_MMCSD_FS_BOOT_PARTITION option was a bit confusing in this context! And I did not realise the behaviour would be to load from a fixed offset. As soon as the tiny printf "issue" is solved, I'll send this patch (as I don't want to break the build). > So yes, please send the above patch, it'd be a nice improvement. > >> (I edited the offset to be able to use u-boot-with-spl.sfp directly) >> >> With these settings, I think we run into the size problem reported >> previously in the ML. >> >> So, enabling tiny printf could help, but we now get: >> >> disk/built-in.o: In function `part_get_info_extended': >> ...u-boot/disk/part_dos.c:236: undefined reference to `sprintf' >> >> So I think we can just put "info->name[0] = 0;" instead of >> the snprintf() calls, when using tiny printf. > > There is more of that stuff in disk/ , at least part_efi.c and > part_iso.c suffer from the exact same problem. I am not sure if > setting the name to '\0' wouldn't break anything, CCing Simon > as he was recently digging in those areas. > > Also, take a look at this patch: > https://patchwork.ozlabs.org/patch/626760/ Ah yes, I naively only looked at the part_dos.c file. Actually a tiny snprintf implementation by Simon was already merged! http://git.denx.de/?p=u-boot.git;a=commit;h=5c411d88be8df5f6a8a1ea0c961f7c35ba82c064 If I implement a naive sprintf() using this as a basis, I can boot from MMC without tweaking anything else, but it adds ~730 bytes to tiny-printf.o. Thanks! Sylvain > >> I can submit those changes as two patches if it makes sense. > > Please do. >
On 05/31/2016 07:43 PM, Sylvain Lesne wrote: > On 05/31/2016 05:42 PM, Marek Vasut wrote: >> On 05/31/2016 04:58 PM, Sylvain Lesne wrote: >>> Hi, >> >> Hi, >> >>> On 05/30/2016 05:22 PM, Marek Vasut wrote: >>>> Enable both features to reduce the SPL size by 6 kiB. >>>> >>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>> Cc: Chin Liang See <clsee@altera.com> >>>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com> >>>> Cc: Pavel Machek <pavel@denx.de> >>>> Cc: Stefan Roese <sr@denx.de> >>> >>> I tried to use the raw MMC boot (on the sockit, with current master >>> + your patches), so I changed the following: >>> >>> --- a/include/configs/socfpga_common.h >>> +++ b/include/configs/socfpga_common.h >>> @@ -349,9 +349,10 @@ unsigned int cm_get_qspi_controller_clk_hz(void); >>> #define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot-dtb.img" >>> #define CONFIG_SPL_LIBDISK_SUPPORT >>> #else >>> -#define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION 3 >>> -#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0xa00 /* offset 2560 >>> sect (1M+256k) */ >>> -#define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS 800 /* 400 KB */ >>> +#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION 3 >>> +#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0x200 /* offset 512 >>> sect (256k) */ >>> +#define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS 1000 /* 512 KB */ >> >> This should be 0x800 (was probably a typo in the original). >> Otherwise this should not break anything, yeah. >> > > I might be missing something obvious, but it actually seems that this > define isn't used outside of the include/configs/ folder... Oh heh, good catch. If you want to nuke that one in include/configs/, be my guest. The mmc spl is now parsing the uImage header to load the right amount of data. >>> +#define CONFIG_SPL_LIBDISK_SUPPORT >>> #endif >>> #endif >>> >>> AFAIK, there were two mistakes: >>> 1) FS_BOOT_PARTITION instead of RAW_MODE_U_BOOT >>> 2) CONFIG_SPL_LIBDISK_SUPPORT was missing >> >> In fact, LIBDISK support was not used before you added the >> CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION into the board config. >> The libdisk functions are needed for determining the start of the >> partition. Looking through the SPL MMC, the code originally loaded the >> u-boot image from a fixed offset on the card (2560 sectors), which is >> indeed not optimal. >> > > I agree, but I thought that the CONFIG_SYS_MMCSD_FS_BOOT_PARTITION > option was a bit confusing in this context! And I did not realise > the behaviour would be to load from a fixed offset. > > As soon as the tiny printf "issue" is solved, I'll send this patch > (as I don't want to break the build). I think I will just pick these patches here, you can apply them and send whatever you have. >> So yes, please send the above patch, it'd be a nice improvement. >> >>> (I edited the offset to be able to use u-boot-with-spl.sfp directly) >>> >>> With these settings, I think we run into the size problem reported >>> previously in the ML. >>> >>> So, enabling tiny printf could help, but we now get: >>> >>> disk/built-in.o: In function `part_get_info_extended': >>> ...u-boot/disk/part_dos.c:236: undefined reference to `sprintf' >>> >>> So I think we can just put "info->name[0] = 0;" instead of >>> the snprintf() calls, when using tiny printf. >> >> There is more of that stuff in disk/ , at least part_efi.c and >> part_iso.c suffer from the exact same problem. I am not sure if >> setting the name to '\0' wouldn't break anything, CCing Simon >> as he was recently digging in those areas. >> >> Also, take a look at this patch: >> https://patchwork.ozlabs.org/patch/626760/ > > Ah yes, I naively only looked at the part_dos.c file. Actually > a tiny snprintf implementation by Simon was already merged! > > http://git.denx.de/?p=u-boot.git;a=commit;h=5c411d88be8df5f6a8a1ea0c961f7c35ba82c064 > > If I implement a naive sprintf() using this as a basis, I can boot > from MMC without tweaking anything else, but it adds ~730 bytes > to tiny-printf.o. I suppose the naive implementation would be calling snprintf() with size = ~0 ? Go for it.
--- a/include/configs/socfpga_common.h +++ b/include/configs/socfpga_common.h @@ -349,9 +349,10 @@ unsigned int cm_get_qspi_controller_clk_hz(void); #define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot-dtb.img" #define CONFIG_SPL_LIBDISK_SUPPORT #else -#define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION 3 -#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0xa00 /* offset 2560 sect (1M+256k) */ -#define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS 800 /* 400 KB */ +#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION 3 +#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0x200 /* offset 512 sect (256k) */ +#define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS 1000 /* 512 KB */ +#define CONFIG_SPL_LIBDISK_SUPPORT #endif