| Message ID | 20190329140910.29503-1-kever.yang@rock-chips.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Philipp Tomsich |
| Headers | show |
| Series | [U-Boot] spl: fit: handle mmc read to sram case in rockchip SoCs | expand |
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); >
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); >> >
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@lists.denx.de > https://lists.denx.de/listinfo/u-boot
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@lists.denx.de <mailto:U-Boot@lists.denx.de> >> https://lists.denx.de/listinfo/u-boot >
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);
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(+)