On 08/29/2012 04:21 PM, Pavel Machek wrote:
> Hi!
>
>>>>> diff --git a/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds
>>>>> b/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds
>>>>
>>>> You should setup MEMORY declarations like the other u-boot-spl linker
>>>> scripts do so we get build-time confirmation that we haven't exceeded
>>>> our size limitations.
>>>
>>> Hmm, I tried, but I don't know socfpga memory layout by heart.
>>>
>>> Dinh, can you help here?
>>
>> I think once you get the answers you should be able to re-post the
>> series cleanly and depend on my v5 (or v6) branch. Thanks!
>
> Ok, figuring the .lds might be tricky. But... that's mostly additional
> (safety) feature. Would it be possible to merge it without that and
> add it later?
>
> Port to v5 of the patch should not be a problem.
>
> For the record and review, current version of patch is here.
A few small comments. No need to re-post intermediate patches, just
address in the next full go-round. Thanks!
> diff --git a/arch/arm/cpu/armv7/socfpga/spl.c
> b/arch/arm/cpu/armv7/socfpga/spl.c
[snip]
> +/*
> + * Board initialization after bss clearance
> + */
> +void spl_board_init(void)
[snip]
> + puts("SPL Boot\n");
Sure you want to keep that line? We already have a general "U-Boot SPL
..." line being printed.
[snip]
> diff --git a/arch/arm/include/asm/arch-socfpga/spl.h
> b/arch/arm/include/asm/arch-socfpga/spl.h
[snip]
> +#define CONFIG_SPL_STACK (&__stack_start)
> +#define CONFIG_SYS_SPL_MALLOC_START (&__malloc_start)
> +#define CONFIG_SYS_SPL_MALLOC_SIZE (&__malloc_end - &__malloc_start)
These should be in the config file.
> diff --git a/board/altera/socfpga_cyclone5/socfpga_cyclone5.c
> b/board/altera/socfpga_cyclone5/socfpga_cyclone5.c
[snip]
> +/*
> + * miscellaneous platform dependent initialisations
> + */
> +int misc_init_r(void)
> +{
> + /* Set to "n" for not verifying the uImage */
> + setenv("verify", "n");
> + return 0;
> +}
Not good to enforce policy like this. Just don't set it in the config
file by default and let users do as they need.
[snip]
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index eaea1c8..5adbf0e 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -78,6 +78,7 @@ void spl_parse_image_header(const struct image_header
> *header)
> u32 header_size = sizeof(struct image_header);
>
> if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
> + /* Valid image. Extract information out of header */
> spl_image.size = __be32_to_cpu(header->ih_size) + header_size;
> spl_image.entry_point = __be32_to_cpu(header->ih_load);
> /* Load including the header */
Just an extra comment, drop please. Or split out if you feel it's
really helpful. No strong opinion here other than not in the same patch
as the rest.
[snip]
> diff --git a/include/common.h b/include/common.h
> index 55025c0..469a3af 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -704,7 +704,9 @@ void external_interrupt (struct pt_regs *);
> void irq_install_handler(int, interrupt_handler_t *, void *);
> void irq_free_handler (int);
> void reset_timer (void);
> -ulong get_timer (ulong base);
> +ulong get_timer(ulong base);
> +void reset_timer_count(void);
> +ulong get_timer_count(ulong base);
> void enable_interrupts (void);
> int disable_interrupts (void);
Make the new declarations match the old, spacing wise please.
>
> diff --git a/include/configs/socfpga_cyclone5.h
> b/include/configs/socfpga_cyclone5.h
> new file mode 100644
> index 0000000..d001cbb
> --- /dev/null
> +++ b/include/configs/socfpga_cyclone5.h
[snip]
> +#undef CONFIG_USE_IRQ
[snip]
> +
> +/*
> + * Stack sizes
> + * The stack sizes are set up in start.S using the settings below
> + */
> +/* regular stack */
> +#define CONFIG_STACKSIZE (128 << 10)
> +#ifdef CONFIG_USE_IRQ
> +/* IRQ stack */
> +#define CONFIG_STACKSIZE_IRQ (4 << 10)
> +/* FIQ stack */
> +#define CONFIG_STACKSIZE_FIQ (4 << 10)
> +#endif
Unused, please drop.
[snip]
> +#define CONFIG_SYS_PROMPT_HUSH_PS2 "> "
This is the default now, can be dropped.
--
Tom
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
