[U-Boot] [PATCH 2/2] arm: socfpga: Enable tiny printf and simple malloc in SPL

[Marek Vasut]

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 at denx.de>
>>>> Cc: Chin Liang See <clsee at altera.com>
>>>> Cc: Dinh Nguyen <dinguyen at opensource.altera.com>
>>>> Cc: Pavel Machek <pavel at denx.de>
>>>> Cc: Stefan Roese <sr at 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.

-- 
Best regards,
Marek Vasut