[U-Boot] [PATCH] spl: atf: fix the plat_params

[U-Boot] [PATCH] spl: atf: fix the plat_params

[Kever Yang]

The latest upstream ATF still not support using a fdt base as plat_params,
I get error like this:
"ERROR:   not expected type found 6410029648624618960"

The reason is the ATF source code parse the plat_param, and can not
decode the type in:
/* common header for all plat parameter type */
struct bl31_plat_param {
	uint64_t type;
	void *next;
};
void params_early_setup(void *plat_param_from_bl2)
plat/rockchip/common/params_setup.c

We can only use the fdt_addr as plat_params after upstream ATF able to
parse it.

BUGFIX to:
1d37909 spl: atf: introduce spl_invoke_atf and make bl31_entry private

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 common/spl/spl_atf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/spl/spl_atf.c b/common/spl/spl_atf.c
index 63557c0..a65d603 100644
--- a/common/spl/spl_atf.c
+++ b/common/spl/spl_atf.c
@@ -96,7 +96,7 @@ static void bl31_entry(uintptr_t bl31_entry, uintptr_t bl33_entry,
 	raw_write_daif(SPSR_EXCEPTION_MASK);
 	dcache_disable();
 
-	atf_entry((void *)bl31_params, (void *)fdt_addr);
+	atf_entry((void *)bl31_params, NULL);
 }
 
 static int spl_fit_images_find_uboot(void *blob)
-- 
1.9.1

[U-Boot] [PATCH] spl: atf: fix the plat_params

[Dr. Philipp Tomsich]

Kever,

If you need/want to disable this, could you make this conditional on a new
Kconfig option?  That way we can disable it for those boards that still ship
with an old ATF (note that passing NULL also broke the upstream ATF for 
quite a number of versions…).

Alternatively, you could add a board-specific wrapper function that allows
modifying the parameters (as they are plat_params, anyway), e.g.:
	atf_entry((void *)bl31_params, board_atf_plat_params(fdt_addr))

This would allow you to suppress this for boards that are known to ship
with a ATF that does not yet support this.

Note that the ATF shipped by us has already been updated, so we can’t
just remove this as it will break functionality for people in the field...

Thanks,
Philipp.

> On 15 Dec 2017, at 04:27, Kever Yang <kever.yang@rock-chips.com> wrote:
> 
> The latest upstream ATF still not support using a fdt base as plat_params,
> I get error like this:
> "ERROR:   not expected type found 6410029648624618960"
> 
> The reason is the ATF source code parse the plat_param, and can not
> decode the type in:
> /* common header for all plat parameter type */
> struct bl31_plat_param {
> 	uint64_t type;
> 	void *next;
> };
> void params_early_setup(void *plat_param_from_bl2)
> plat/rockchip/common/params_setup.c
> 
> We can only use the fdt_addr as plat_params after upstream ATF able to
> parse it.
> 
> BUGFIX to:
> 1d37909 spl: atf: introduce spl_invoke_atf and make bl31_entry private
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
> 
> common/spl/spl_atf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/spl/spl_atf.c b/common/spl/spl_atf.c
> index 63557c0..a65d603 100644
> --- a/common/spl/spl_atf.c
> +++ b/common/spl/spl_atf.c
> @@ -96,7 +96,7 @@ static void bl31_entry(uintptr_t bl31_entry, uintptr_t bl33_entry,
> 	raw_write_daif(SPSR_EXCEPTION_MASK);
> 	dcache_disable();
> 
> -	atf_entry((void *)bl31_params, (void *)fdt_addr);
> +	atf_entry((void *)bl31_params, NULL);
> }
> 
> static int spl_fit_images_find_uboot(void *blob)
> -- 
> 1.9.1
> 

[U-Boot] [PATCH] spl: atf: fix the plat_params

[Kever Yang]

Hi Philipp,


On 12/15/2017 05:07 PM, Dr. Philipp Tomsich wrote:
> Kever,
>
> If you need/want to disable this, could you make this conditional on a new
> Kconfig option?  That way we can disable it for those boards that still ship
> with an old ATF

That's not an "old" ATF, it's the latest upstream version and wildly 
used by all
the open source community. Many people get from upstream first, and then
other forks.
> (note that passing NULL also broke the upstream ATF for
> quite a number of versions…).

A NULL pointer means no parameter for plat_params, and it will not broke 
upstream ATF.

Well, what make me crazy is that every time I update a version from 
upstream U-Boot,
I have to debug for different issues on different boards:(

> Alternatively, you could add a board-specific wrapper function that allows
> modifying the parameters (as they are plat_params, anyway), e.g.:
> 	atf_entry((void *)bl31_params, board_atf_plat_params(fdt_addr))
>
> This would allow you to suppress this for boards that are known to ship
> with a ATF that does not yet support this.

We should do this before previous patch, right?
>
> Note that the ATF shipped by us has already been updated, so we can’t
> just remove this as it will break functionality for people in the field...

You customer should get a complete version with all functionality work 
from your
server, right? It's not totally the same as the upstream version, just 
like Rockchip have
a version on github for evbs, which have been test by our QA.

BUT, open source community always get a BROKEN version from upstream :(
The upstream source code should have a good support for the boards 
already upstream,
but it broken very frequently.

Thanks,
- Kever
>
> Thanks,
> Philipp.
>
>> On 15 Dec 2017, at 04:27, Kever Yang <kever.yang@rock-chips.com> wrote:
>>
>> The latest upstream ATF still not support using a fdt base as plat_params,
>> I get error like this:
>> "ERROR:   not expected type found 6410029648624618960"
>>
>> The reason is the ATF source code parse the plat_param, and can not
>> decode the type in:
>> /* common header for all plat parameter type */
>> struct bl31_plat_param {
>> 	uint64_t type;
>> 	void *next;
>> };
>> void params_early_setup(void *plat_param_from_bl2)
>> plat/rockchip/common/params_setup.c
>>
>> We can only use the fdt_addr as plat_params after upstream ATF able to
>> parse it.
>>
>> BUGFIX to:
>> 1d37909 spl: atf: introduce spl_invoke_atf and make bl31_entry private
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>> common/spl/spl_atf.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/spl/spl_atf.c b/common/spl/spl_atf.c
>> index 63557c0..a65d603 100644
>> --- a/common/spl/spl_atf.c
>> +++ b/common/spl/spl_atf.c
>> @@ -96,7 +96,7 @@ static void bl31_entry(uintptr_t bl31_entry, uintptr_t bl33_entry,
>> 	raw_write_daif(SPSR_EXCEPTION_MASK);
>> 	dcache_disable();
>>
>> -	atf_entry((void *)bl31_params, (void *)fdt_addr);
>> +	atf_entry((void *)bl31_params, NULL);
>> }
>>
>> static int spl_fit_images_find_uboot(void *blob)
>> -- 
>> 1.9.1
>>
>

[U-Boot] [PATCH] spl: atf: fix the plat_params

[Kever Yang]

Hi Philipp,

     I have ask Rockchip ATF maintainers to update both internal and 
upstream

ATF to support the pointer to FDT for plat_params.

     But I really hope people don't break the software already in 
use/upstream

so easily, we need to update every related module in stead of break others.

Or else people would not like to use source code from U-Boot upstream.


Thanks,
- Kever
On 12/19/2017 10:03 AM, Kever Yang wrote:
> Hi Philipp,
>
>
> On 12/15/2017 05:07 PM, Dr. Philipp Tomsich wrote:
>> Kever,
>>
>> If you need/want to disable this, could you make this conditional on 
>> a new
>> Kconfig option?  That way we can disable it for those boards that 
>> still ship
>> with an old ATF
>
> That's not an "old" ATF, it's the latest upstream version and wildly 
> used by all
> the open source community. Many people get from upstream first, and then
> other forks.
>> (note that passing NULL also broke the upstream ATF for
>> quite a number of versions…).
>
> A NULL pointer means no parameter for plat_params, and it will not 
> broke upstream ATF.
>
> Well, what make me crazy is that every time I update a version from 
> upstream U-Boot,
> I have to debug for different issues on different boards:(
>
>> Alternatively, you could add a board-specific wrapper function that 
>> allows
>> modifying the parameters (as they are plat_params, anyway), e.g.:
>>     atf_entry((void *)bl31_params, board_atf_plat_params(fdt_addr))
>>
>> This would allow you to suppress this for boards that are known to ship
>> with a ATF that does not yet support this.
>
> We should do this before previous patch, right?
>>
>> Note that the ATF shipped by us has already been updated, so we can’t
>> just remove this as it will break functionality for people in the 
>> field...
>
> You customer should get a complete version with all functionality work 
> from your
> server, right? It's not totally the same as the upstream version, just 
> like Rockchip have
> a version on github for evbs, which have been test by our QA.
>
> BUT, open source community always get a BROKEN version from upstream :(
> The upstream source code should have a good support for the boards 
> already upstream,
> but it broken very frequently.
>
> Thanks,
> - Kever
>>
>> Thanks,
>> Philipp.
>>
>>> On 15 Dec 2017, at 04:27, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>
>>> The latest upstream ATF still not support using a fdt base as 
>>> plat_params,
>>> I get error like this:
>>> "ERROR:   not expected type found 6410029648624618960"
>>>
>>> The reason is the ATF source code parse the plat_param, and can not
>>> decode the type in:
>>> /* common header for all plat parameter type */
>>> struct bl31_plat_param {
>>>     uint64_t type;
>>>     void *next;
>>> };
>>> void params_early_setup(void *plat_param_from_bl2)
>>> plat/rockchip/common/params_setup.c
>>>
>>> We can only use the fdt_addr as plat_params after upstream ATF able to
>>> parse it.
>>>
>>> BUGFIX to:
>>> 1d37909 spl: atf: introduce spl_invoke_atf and make bl31_entry private
>>>
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>> ---
>>>
>>> common/spl/spl_atf.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/common/spl/spl_atf.c b/common/spl/spl_atf.c
>>> index 63557c0..a65d603 100644
>>> --- a/common/spl/spl_atf.c
>>> +++ b/common/spl/spl_atf.c
>>> @@ -96,7 +96,7 @@ static void bl31_entry(uintptr_t bl31_entry, 
>>> uintptr_t bl33_entry,
>>>     raw_write_daif(SPSR_EXCEPTION_MASK);
>>>     dcache_disable();
>>>
>>> -    atf_entry((void *)bl31_params, (void *)fdt_addr);
>>> +    atf_entry((void *)bl31_params, NULL);
>>> }
>>>
>>> static int spl_fit_images_find_uboot(void *blob)
>>> -- 
>>> 1.9.1
>>>
>>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot