[U-Boot,1/6] arm: socfpga: fix SPL on gen5 after moving to DM serial

Message ID 20180809190420.28093-2-simon.k.r.goldschmidt@gmail.com
State Superseded
Delegated to: Marek Vasut
Headers show
Series
  • Get socfpga gen5 SPL working again.
Related show

Commit Message

Simon Goldschmidt Aug. 9, 2018, 7:04 p.m.
There were NULL pointers dereferenced because DM was used
too early without correct initialization:
- malloc_simple returned NULL when called from preloader_console_init()
  because gd->malloc_limit was 0
- uclass_add dereferenced gd->uclass_root members which were NULL because
  dm_init (or one of its relatives) has not been called.

All this is fixed by calling spl_early_init before calling
preloader_console_init.

This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial")

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---
v2:
- Don't remove gd->malloc_base assignment at the end of board_init_f()
  (moved to an extra patch)

 arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Marek Vasut Aug. 9, 2018, 9:42 p.m. | #1
On 08/09/2018 09:04 PM, Simon Goldschmidt wrote:
> There were NULL pointers dereferenced because DM was used
> too early without correct initialization:
> - malloc_simple returned NULL when called from preloader_console_init()
>   because gd->malloc_limit was 0
> - uclass_add dereferenced gd->uclass_root members which were NULL because
>   dm_init (or one of its relatives) has not been called.
> 
> All this is fixed by calling spl_early_init before calling
> preloader_console_init.
> 
> This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial")
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> v2:
> - Don't remove gd->malloc_base assignment at the end of board_init_f()
>   (moved to an extra patch)
> 
>  arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c
> index d6fe7d35af..9bdfaa3c1e 100644
> --- a/arch/arm/mach-socfpga/spl_gen5.c
> +++ b/arch/arm/mach-socfpga/spl_gen5.c
> @@ -86,6 +86,7 @@ void board_init_f(ulong dummy)
>  	const struct cm_config *cm_default_cfg = cm_get_default_config();
>  	unsigned long sdram_size;
>  	unsigned long reg;
> +	int ret;
>  
>  	/*
>  	 * First C code to run. Clear fake OCRAM ECC first as SBE
> @@ -152,6 +153,12 @@ void board_init_f(ulong dummy)
>  	/* unfreeze / thaw all IO banks */
>  	sys_mgr_frzctrl_thaw_req();
>  
> +	ret = spl_early_init();

Uh, but isn't this called from common/spl/spl.c ? I suspect the SoCFPGA
SPL is a bit weird.

> +	if (ret) {
> +		debug("spl_early_init() failed: %d\n", ret);
> +		hang();
> +	}
> +
>  	/* enable console uart printing */
>  	preloader_console_init();
>  
>
Simon Goldschmidt Aug. 10, 2018, 12:39 p.m. | #2
On 09.08.2018 23:42, Marek Vasut wrote:
> On 08/09/2018 09:04 PM, Simon Goldschmidt wrote:
>> There were NULL pointers dereferenced because DM was used
>> too early without correct initialization:
>> - malloc_simple returned NULL when called from preloader_console_init()
>>    because gd->malloc_limit was 0
>> - uclass_add dereferenced gd->uclass_root members which were NULL because
>>    dm_init (or one of its relatives) has not been called.
>>
>> All this is fixed by calling spl_early_init before calling
>> preloader_console_init.
>>
>> This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial")
>>
>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>> ---
>> v2:
>> - Don't remove gd->malloc_base assignment at the end of board_init_f()
>>    (moved to an extra patch)
>>
>>   arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c
>> index d6fe7d35af..9bdfaa3c1e 100644
>> --- a/arch/arm/mach-socfpga/spl_gen5.c
>> +++ b/arch/arm/mach-socfpga/spl_gen5.c
>> @@ -86,6 +86,7 @@ void board_init_f(ulong dummy)
>>   	const struct cm_config *cm_default_cfg = cm_get_default_config();
>>   	unsigned long sdram_size;
>>   	unsigned long reg;
>> +	int ret;
>>   
>>   	/*
>>   	 * First C code to run. Clear fake OCRAM ECC first as SBE
>> @@ -152,6 +153,12 @@ void board_init_f(ulong dummy)
>>   	/* unfreeze / thaw all IO banks */
>>   	sys_mgr_frzctrl_thaw_req();
>>   
>> +	ret = spl_early_init();
> Uh, but isn't this called from common/spl/spl.c ? I suspect the SoCFPGA
> SPL is a bit weird.

Ehrm, I copied this from spl_s10.c, but other boards seem to do this, 
too. Honestly, I don't know how any SPL can use DM serial without this 
being called. Maybe other SPLs initialize the serial port later (not in 
board_init_f).

common/spl/spl.c calls spl_init(), which also calls the part that 
spl_early_init() calls.

I can only take other SPLs as reference and from reading all the code, I 
think this should be good.

Simon

>
>> +	if (ret) {
>> +		debug("spl_early_init() failed: %d\n", ret);
>> +		hang();
>> +	}
>> +
>>   	/* enable console uart printing */
>>   	preloader_console_init();
>>   
>>
>
Marek Vasut Aug. 10, 2018, 12:41 p.m. | #3
On 08/10/2018 02:39 PM, Simon Goldschmidt wrote:
> On 09.08.2018 23:42, Marek Vasut wrote:
>> On 08/09/2018 09:04 PM, Simon Goldschmidt wrote:
>>> There were NULL pointers dereferenced because DM was used
>>> too early without correct initialization:
>>> - malloc_simple returned NULL when called from preloader_console_init()
>>>    because gd->malloc_limit was 0
>>> - uclass_add dereferenced gd->uclass_root members which were NULL
>>> because
>>>    dm_init (or one of its relatives) has not been called.
>>>
>>> All this is fixed by calling spl_early_init before calling
>>> preloader_console_init.
>>>
>>> This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial")
>>>
>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>> ---
>>> v2:
>>> - Don't remove gd->malloc_base assignment at the end of board_init_f()
>>>    (moved to an extra patch)
>>>
>>>   arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-socfpga/spl_gen5.c
>>> b/arch/arm/mach-socfpga/spl_gen5.c
>>> index d6fe7d35af..9bdfaa3c1e 100644
>>> --- a/arch/arm/mach-socfpga/spl_gen5.c
>>> +++ b/arch/arm/mach-socfpga/spl_gen5.c
>>> @@ -86,6 +86,7 @@ void board_init_f(ulong dummy)
>>>       const struct cm_config *cm_default_cfg = cm_get_default_config();
>>>       unsigned long sdram_size;
>>>       unsigned long reg;
>>> +    int ret;
>>>         /*
>>>        * First C code to run. Clear fake OCRAM ECC first as SBE
>>> @@ -152,6 +153,12 @@ void board_init_f(ulong dummy)
>>>       /* unfreeze / thaw all IO banks */
>>>       sys_mgr_frzctrl_thaw_req();
>>>   +    ret = spl_early_init();
>> Uh, but isn't this called from common/spl/spl.c ? I suspect the SoCFPGA
>> SPL is a bit weird.
> 
> Ehrm, I copied this from spl_s10.c, but other boards seem to do this,
> too. Honestly, I don't know how any SPL can use DM serial without this
> being called. Maybe other SPLs initialize the serial port later (not in
> board_init_f).

I mean, spl_early_init() is called in common/spl/spl.c , which is common
code. Maybe the socfpga SPL is structured in a really weird way (I think
it is).

> common/spl/spl.c calls spl_init(), which also calls the part that
> spl_early_init() calls.
> 
> I can only take other SPLs as reference and from reading all the code, I
> think this should be good.

Right

> Simon
> 
>>
>>> +    if (ret) {
>>> +        debug("spl_early_init() failed: %d\n", ret);
>>> +        hang();
>>> +    }
>>> +
>>>       /* enable console uart printing */
>>>       preloader_console_init();
>>>  
>>
>
Simon Goldschmidt Aug. 10, 2018, 12:55 p.m. | #4
On 10.08.2018 14:41, Marek Vasut wrote:
> On 08/10/2018 02:39 PM, Simon Goldschmidt wrote:
>> On 09.08.2018 23:42, Marek Vasut wrote:
>>> On 08/09/2018 09:04 PM, Simon Goldschmidt wrote:
>>>> There were NULL pointers dereferenced because DM was used
>>>> too early without correct initialization:
>>>> - malloc_simple returned NULL when called from preloader_console_init()
>>>>     because gd->malloc_limit was 0
>>>> - uclass_add dereferenced gd->uclass_root members which were NULL
>>>> because
>>>>     dm_init (or one of its relatives) has not been called.
>>>>
>>>> All this is fixed by calling spl_early_init before calling
>>>> preloader_console_init.
>>>>
>>>> This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial")
>>>>
>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>>> ---
>>>> v2:
>>>> - Don't remove gd->malloc_base assignment at the end of board_init_f()
>>>>     (moved to an extra patch)
>>>>
>>>>    arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/arm/mach-socfpga/spl_gen5.c
>>>> b/arch/arm/mach-socfpga/spl_gen5.c
>>>> index d6fe7d35af..9bdfaa3c1e 100644
>>>> --- a/arch/arm/mach-socfpga/spl_gen5.c
>>>> +++ b/arch/arm/mach-socfpga/spl_gen5.c
>>>> @@ -86,6 +86,7 @@ void board_init_f(ulong dummy)
>>>>        const struct cm_config *cm_default_cfg = cm_get_default_config();
>>>>        unsigned long sdram_size;
>>>>        unsigned long reg;
>>>> +    int ret;
>>>>          /*
>>>>         * First C code to run. Clear fake OCRAM ECC first as SBE
>>>> @@ -152,6 +153,12 @@ void board_init_f(ulong dummy)
>>>>        /* unfreeze / thaw all IO banks */
>>>>        sys_mgr_frzctrl_thaw_req();
>>>>    +    ret = spl_early_init();
>>> Uh, but isn't this called from common/spl/spl.c ? I suspect the SoCFPGA
>>> SPL is a bit weird.
>> Ehrm, I copied this from spl_s10.c, but other boards seem to do this,
>> too. Honestly, I don't know how any SPL can use DM serial without this
>> being called. Maybe other SPLs initialize the serial port later (not in
>> board_init_f).
> I mean, spl_early_init() is called in common/spl/spl.c , which is common
> code. Maybe the socfpga SPL is structured in a really weird way (I think
> it is).

Not exactly: common/spl/spl.c calls spl_common_init(), just like 
spl_early_init() does. Given the names, I think spl_early_init() is 
meant to be called early, e.g. from board_init_f() ;-)

Oh, I just saw spl_common_init() emits a debug print "spl_early_init()", 
so that might have tricked you...?

>> common/spl/spl.c calls spl_init(), which also calls the part that
>> spl_early_init() calls.
>>
>> I can only take other SPLs as reference and from reading all the code, I
>> think this should be good.
> Right

So is this change OK for v2018.09 once I fix the dts thing? Given that 
v2018.07 is broken for socfpga gen5, it would be good to merge it 
before. I can prepare a v3 of the series with only minimal changes in 
the socfpga files and resend the rest as detached patches.

Simon

>>>> +    if (ret) {
>>>> +        debug("spl_early_init() failed: %d\n", ret);
>>>> +        hang();
>>>> +    }
>>>> +
>>>>        /* enable console uart printing */
>>>>        preloader_console_init();
>>>>   
>
Marek Vasut Aug. 10, 2018, 1:12 p.m. | #5
On 08/10/2018 02:55 PM, Simon Goldschmidt wrote:
> On 10.08.2018 14:41, Marek Vasut wrote:
>> On 08/10/2018 02:39 PM, Simon Goldschmidt wrote:
>>> On 09.08.2018 23:42, Marek Vasut wrote:
>>>> On 08/09/2018 09:04 PM, Simon Goldschmidt wrote:
>>>>> There were NULL pointers dereferenced because DM was used
>>>>> too early without correct initialization:
>>>>> - malloc_simple returned NULL when called from
>>>>> preloader_console_init()
>>>>>     because gd->malloc_limit was 0
>>>>> - uclass_add dereferenced gd->uclass_root members which were NULL
>>>>> because
>>>>>     dm_init (or one of its relatives) has not been called.
>>>>>
>>>>> All this is fixed by calling spl_early_init before calling
>>>>> preloader_console_init.
>>>>>
>>>>> This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial")
>>>>>
>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>>>> ---
>>>>> v2:
>>>>> - Don't remove gd->malloc_base assignment at the end of board_init_f()
>>>>>     (moved to an extra patch)
>>>>>
>>>>>    arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++
>>>>>    1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/mach-socfpga/spl_gen5.c
>>>>> b/arch/arm/mach-socfpga/spl_gen5.c
>>>>> index d6fe7d35af..9bdfaa3c1e 100644
>>>>> --- a/arch/arm/mach-socfpga/spl_gen5.c
>>>>> +++ b/arch/arm/mach-socfpga/spl_gen5.c
>>>>> @@ -86,6 +86,7 @@ void board_init_f(ulong dummy)
>>>>>        const struct cm_config *cm_default_cfg =
>>>>> cm_get_default_config();
>>>>>        unsigned long sdram_size;
>>>>>        unsigned long reg;
>>>>> +    int ret;
>>>>>          /*
>>>>>         * First C code to run. Clear fake OCRAM ECC first as SBE
>>>>> @@ -152,6 +153,12 @@ void board_init_f(ulong dummy)
>>>>>        /* unfreeze / thaw all IO banks */
>>>>>        sys_mgr_frzctrl_thaw_req();
>>>>>    +    ret = spl_early_init();
>>>> Uh, but isn't this called from common/spl/spl.c ? I suspect the SoCFPGA
>>>> SPL is a bit weird.
>>> Ehrm, I copied this from spl_s10.c, but other boards seem to do this,
>>> too. Honestly, I don't know how any SPL can use DM serial without this
>>> being called. Maybe other SPLs initialize the serial port later (not in
>>> board_init_f).
>> I mean, spl_early_init() is called in common/spl/spl.c , which is common
>> code. Maybe the socfpga SPL is structured in a really weird way (I think
>> it is).
> 
> Not exactly: common/spl/spl.c calls spl_common_init(), just like
> spl_early_init() does. Given the names, I think spl_early_init() is
> meant to be called early, e.g. from board_init_f() ;-)
> 
> Oh, I just saw spl_common_init() emits a debug print "spl_early_init()",
> so that might have tricked you...?

Ah, yes, I think so.

>>> common/spl/spl.c calls spl_init(), which also calls the part that
>>> spl_early_init() calls.
>>>
>>> I can only take other SPLs as reference and from reading all the code, I
>>> think this should be good.
>> Right
> 
> So is this change OK for v2018.09 once I fix the dts thing? Given that
> v2018.07 is broken for socfpga gen5, it would be good to merge it
> before. I can prepare a v3 of the series with only minimal changes in
> the socfpga files and resend the rest as detached patches.
Looks good, yes.

Patch

diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c
index d6fe7d35af..9bdfaa3c1e 100644
--- a/arch/arm/mach-socfpga/spl_gen5.c
+++ b/arch/arm/mach-socfpga/spl_gen5.c
@@ -86,6 +86,7 @@  void board_init_f(ulong dummy)
 	const struct cm_config *cm_default_cfg = cm_get_default_config();
 	unsigned long sdram_size;
 	unsigned long reg;
+	int ret;
 
 	/*
 	 * First C code to run. Clear fake OCRAM ECC first as SBE
@@ -152,6 +153,12 @@  void board_init_f(ulong dummy)
 	/* unfreeze / thaw all IO banks */
 	sys_mgr_frzctrl_thaw_req();
 
+	ret = spl_early_init();
+	if (ret) {
+		debug("spl_early_init() failed: %d\n", ret);
+		hang();
+	}
+
 	/* enable console uart printing */
 	preloader_console_init();