[U-Boot] [PATCH] spl: fit: handle mmc read to sram case in rockchip SoCs

[U-Boot] [PATCH] spl: fit: handle mmc read to sram case in rockchip SoCs

[Kever Yang]

Rockchip fit image with atf may have firmware for sram,
so the fit driver need to read data from mmc to sram,
but Rockchip mmc controller does not support this data
path, we have to read into ddr first and then copy it
to sram.

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

 common/spl/spl_fit.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index c9bfe0cc8a..5c5b58f69d 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -9,6 +9,7 @@
 #include <fpga.h>
 #include <image.h>
 #include <linux/libfdt.h>
+#include <malloc.h>
 #include <spl.h>
 
 #ifndef CONFIG_SYS_BOOTM_LEN
@@ -215,6 +216,15 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 			return -ENOENT;
 
 		load_ptr = (load_addr + align_len) & ~align_len;
+#if  defined(CONFIG_ROCKCHIP_RK3399) || defined(CONFIG_ROCKCHIP_RK3368)
+		/*
+		 * Rockchip SOC's mmc controller does not support read data
+		 * from mmc to sram, we have to read to sdram first, and then
+		 * copy to sram.
+		 */
+		if ((load_ptr & 0xffff0000) != CONFIG_SYS_SDRAM_BASE)
+			load_ptr = (ulong)memalign(ARCH_DMA_MINALIGN, len);
+#endif
 		length = len;
 
 		overhead = get_aligned_image_overhead(info, offset);
-- 
2.20.1

[U-Boot] [PATCH] spl: fit: handle mmc read to sram case in rockchip SoCs

[Simon Goldschmidt]

On 29.03.19 15:09, Kever Yang wrote:
> Rockchip fit image with atf may have firmware for sram,
> so the fit driver need to read data from mmc to sram,
> but Rockchip mmc controller does not support this data
> path, we have to read into ddr first and then copy it
> to sram.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
> 
>   common/spl/spl_fit.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index c9bfe0cc8a..5c5b58f69d 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -9,6 +9,7 @@
>   #include <fpga.h>
>   #include <image.h>
>   #include <linux/libfdt.h>
> +#include <malloc.h>
>   #include <spl.h>
>   
>   #ifndef CONFIG_SYS_BOOTM_LEN
> @@ -215,6 +216,15 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>   			return -ENOENT;
>   
>   		load_ptr = (load_addr + align_len) & ~align_len;
> +#if  defined(CONFIG_ROCKCHIP_RK3399) || defined(CONFIG_ROCKCHIP_RK3368)

This looks hacky. I don't think we should clutter platform independent 
code with platform dependent ifdefs. You're totally violating code 
abstraction here.

Regards,
Simon

> +		/*
> +		 * Rockchip SOC's mmc controller does not support read data
> +		 * from mmc to sram, we have to read to sdram first, and then
> +		 * copy to sram.
> +		 */
> +		if ((load_ptr & 0xffff0000) != CONFIG_SYS_SDRAM_BASE)
> +			load_ptr = (ulong)memalign(ARCH_DMA_MINALIGN, len);
> +#endif
>   		length = len;
>   
>   		overhead = get_aligned_image_overhead(info, offset);
> 

[U-Boot] [PATCH] spl: fit: handle mmc read to sram case in rockchip SoCs

[Kever Yang]

Hi Simon,


On 03/29/2019 11:33 PM, Simon Goldschmidt wrote:
>
>
> On 29.03.19 15:09, Kever Yang wrote:
>> Rockchip fit image with atf may have firmware for sram,
>> so the fit driver need to read data from mmc to sram,
>> but Rockchip mmc controller does not support this data
>> path, we have to read into ddr first and then copy it
>> to sram.
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>>   common/spl/spl_fit.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index c9bfe0cc8a..5c5b58f69d 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -9,6 +9,7 @@
>>   #include <fpga.h>
>>   #include <image.h>
>>   #include <linux/libfdt.h>
>> +#include <malloc.h>
>>   #include <spl.h>
>>     #ifndef CONFIG_SYS_BOOTM_LEN
>> @@ -215,6 +216,15 @@ static int spl_load_fit_image(struct
>> spl_load_info *info, ulong sector,
>>               return -ENOENT;
>>             load_ptr = (load_addr + align_len) & ~align_len;
>> +#if  defined(CONFIG_ROCKCHIP_RK3399) || defined(CONFIG_ROCKCHIP_RK3368)
>
> This looks hacky. I don't think we should clutter platform independent
> code with platform dependent ifdefs. You're totally violating code
> abstraction here.
>

Thanks for your comment, I know this looks hacky, could you share you
idea about how to implement this better?
We do need this to make things work.

Thanks,
- Kever
> Regards,
> Simon
>
>> +        /*
>> +         * Rockchip SOC's mmc controller does not support read data
>> +         * from mmc to sram, we have to read to sdram first, and then
>> +         * copy to sram.
>> +         */
>> +        if ((load_ptr & 0xffff0000) != CONFIG_SYS_SDRAM_BASE)
>> +            load_ptr = (ulong)memalign(ARCH_DMA_MINALIGN, len);
>> +#endif
>>           length = len;
>>             overhead = get_aligned_image_overhead(info, offset);
>>
>

[U-Boot] [PATCH] spl: fit: handle mmc read to sram case in rockchip SoCs

[Philipp Tomsich]

Kever,

> On 30.03.2019, at 02:43, Kever Yang <kever.yang@rock-chips.com> wrote:
> 
> Hi Simon,
> 
> 
> On 03/29/2019 11:33 PM, Simon Goldschmidt wrote:
>> 
>> 
>> On 29.03.19 15:09, Kever Yang wrote:
>>> Rockchip fit image with atf may have firmware for sram,
>>> so the fit driver need to read data from mmc to sram,
>>> but Rockchip mmc controller does not support this data
>>> path, we have to read into ddr first and then copy it
>>> to sram.
>>> 
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>> ---
>>> 
>>>   common/spl/spl_fit.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>> 
>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>> index c9bfe0cc8a..5c5b58f69d 100644
>>> --- a/common/spl/spl_fit.c
>>> +++ b/common/spl/spl_fit.c
>>> @@ -9,6 +9,7 @@
>>>   #include <fpga.h>
>>>   #include <image.h>
>>>   #include <linux/libfdt.h>
>>> +#include <malloc.h>
>>>   #include <spl.h>
>>>     #ifndef CONFIG_SYS_BOOTM_LEN
>>> @@ -215,6 +216,15 @@ static int spl_load_fit_image(struct
>>> spl_load_info *info, ulong sector,
>>>               return -ENOENT;
>>>             load_ptr = (load_addr + align_len) & ~align_len;
>>> +#if  defined(CONFIG_ROCKCHIP_RK3399) || defined(CONFIG_ROCKCHIP_RK3368)
>> 
>> This looks hacky. I don't think we should clutter platform independent
>> code with platform dependent ifdefs. You're totally violating code
>> abstraction here.
>> 
> 
> Thanks for your comment, I know this looks hacky, could you share you
> idea about how to implement this better?
> We do need this to make things work.

A more appropriate way would be to leverage the bounce buffers in the SD/MMC
drivers and to make sure that the 0xffff0000 memory region is not allocated as a
DMA-able target address via bounce buffers.

Note that this is the same issue that I had highlighted about a year and a half ago
and that we ended up working around by loading the PMU firmware into DRAM,
pass the info about the loadables to ATF as part of the FDT, and finally have ATF
relocate the loadables to INTMEM.

> Thanks,
> - Kever
>> Regards,
>> Simon
>> 
>>> +        /*
>>> +         * Rockchip SOC's mmc controller does not support read data
>>> +         * from mmc to sram, we have to read to sdram first, and then
>>> +         * copy to sram.
>>> +         */
>>> +        if ((load_ptr & 0xffff0000) != CONFIG_SYS_SDRAM_BASE)
>>> +            load_ptr = (ulong)memalign(ARCH_DMA_MINALIGN, len);
>>> +#endif
>>>           length = len;
>>>             overhead = get_aligned_image_overhead(info, offset);
>>> 
>> 
> 
> 
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] spl: fit: handle mmc read to sram case in rockchip SoCs

[Kever Yang]

Hi Philipp,


On 03/30/2019 06:16 PM, Philipp Tomsich wrote:
> Kever,
>
>> On 30.03.2019, at 02:43, Kever Yang <kever.yang@rock-chips.com
>> <mailto:kever.yang@rock-chips.com>> wrote:
>>
>> Hi Simon,
>>
>>
>> On 03/29/2019 11:33 PM, Simon Goldschmidt wrote:
>>>
>>>
>>> On 29.03.19 15:09, Kever Yang wrote:
>>>> Rockchip fit image with atf may have firmware for sram,
>>>> so the fit driver need to read data from mmc to sram,
>>>> but Rockchip mmc controller does not support this data
>>>> path, we have to read into ddr first and then copy it
>>>> to sram.
>>>>
>>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com
>>>> <mailto:kever.yang@rock-chips.com>>
>>>> ---
>>>>
>>>>   common/spl/spl_fit.c | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>>> index c9bfe0cc8a..5c5b58f69d 100644
>>>> --- a/common/spl/spl_fit.c
>>>> +++ b/common/spl/spl_fit.c
>>>> @@ -9,6 +9,7 @@
>>>>   #include <fpga.h>
>>>>   #include <image.h>
>>>>   #include <linux/libfdt.h>
>>>> +#include <malloc.h>
>>>>   #include <spl.h>
>>>>     #ifndef CONFIG_SYS_BOOTM_LEN
>>>> @@ -215,6 +216,15 @@ static int spl_load_fit_image(struct
>>>> spl_load_info *info, ulong sector,
>>>>               return -ENOENT;
>>>>             load_ptr = (load_addr + align_len) & ~align_len;
>>>> +#if  defined(CONFIG_ROCKCHIP_RK3399) ||
>>>> defined(CONFIG_ROCKCHIP_RK3368)
>>>
>>> This looks hacky. I don't think we should clutter platform independent
>>> code with platform dependent ifdefs. You're totally violating code
>>> abstraction here.
>>>
>>
>> Thanks for your comment, I know this looks hacky, could you share you
>> idea about how to implement this better?
>> We do need this to make things work.
>
> A more appropriate way would be to leverage the bounce buffers in the
> SD/MMC
> drivers and to make sure that the 0xffff0000 memory region is not
> allocated as a
> DMA-able target address via bounce buffers.

Bounce buffer is a good idea, I have implement it and send to patch to
the list, thanks
very much.

This patch can be drop.

Thanks,
- Kever
>
> Note that this is the same issue that I had highlighted about a year
> and a half ago
> and that we ended up working around by loading the PMU firmware into DRAM,
> pass the info about the loadables to ATF as part of the FDT, and
> finally have ATF
> relocate the loadables to INTMEM.
>
>> Thanks,
>> - Kever
>>> Regards,
>>> Simon
>>>
>>>> +        /*
>>>> +         * Rockchip SOC's mmc controller does not support read data
>>>> +         * from mmc to sram, we have to read to sdram first, and then
>>>> +         * copy to sram.
>>>> +         */
>>>> +        if ((load_ptr & 0xffff0000) != CONFIG_SYS_SDRAM_BASE)
>>>> +            load_ptr = (ulong)memalign(ARCH_DMA_MINALIGN, len);
>>>> +#endif
>>>>           length = len;
>>>>             overhead = get_aligned_image_overhead(info, offset);
>>>>
>>>
>>
>>
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de <mailto:U-Boot@lists.denx.de>
>> https://lists.denx.de/listinfo/u-boot
>