| Message ID | 1459755138-173508-1-git-send-email-agraf@suse.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
On 04/04/2016 09:32 AM, Alexander Graf wrote: > The cache line flush helpers only work properly when they get aligned > start and end addresses. Round our flush range to cache line size. It's > safe because we're guaranteed to flush within a single page which has the > same cache attributes. > > Reported-by: Marek Vasut <marex@denx.de> > Signed-off-by: Alexander Graf <agraf@suse.de> Thanks!
Am 04.04.2016 um 09:32 schrieb Alexander Graf: > The cache line flush helpers only work properly when they get aligned > start and end addresses. Round our flush range to cache line size. It's > safe because we're guaranteed to flush within a single page which has the > same cache attributes. > > Reported-by: Marek Vasut <marex@denx.de> > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > lib/efi_loader/efi_runtime.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c > index 22bcd08..40acec0 100644 > --- a/lib/efi_loader/efi_runtime.c > +++ b/lib/efi_loader/efi_runtime.c > @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) > #endif > > *p = newaddr; > - flush_dcache_range((ulong)p, (ulong)&p[1]); > + flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1), > + ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE)); dragonboard410c_defconfig fails to build with this due to undefined CONFIG_SYS_CACHELINE_SIZE. Do we need to #ifdef here or is the dragonboard410c at fault for not defining it? jetson-tk1_defconfig for instance builds fine with this patch. Regards, Andreas > } > > #ifndef IS_RELA
On 11.04.16 00:24, Andreas Färber wrote: > Am 04.04.2016 um 09:32 schrieb Alexander Graf: >> The cache line flush helpers only work properly when they get aligned >> start and end addresses. Round our flush range to cache line size. It's >> safe because we're guaranteed to flush within a single page which has the >> same cache attributes. >> >> Reported-by: Marek Vasut <marex@denx.de> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> --- >> lib/efi_loader/efi_runtime.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c >> index 22bcd08..40acec0 100644 >> --- a/lib/efi_loader/efi_runtime.c >> +++ b/lib/efi_loader/efi_runtime.c >> @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) >> #endif >> >> *p = newaddr; >> - flush_dcache_range((ulong)p, (ulong)&p[1]); >> + flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1), >> + ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE)); > > dragonboard410c_defconfig fails to build with this due to undefined > CONFIG_SYS_CACHELINE_SIZE. Do we need to #ifdef here or is the > dragonboard410c at fault for not defining it? Some USB drivers use it unconditionally, but I guess the dragonboard401c doesn't enable USB. Let me come up with a more compatible version. Alex
On 04/11/2016 12:31 AM, Alexander Graf wrote: > > > On 11.04.16 00:24, Andreas Färber wrote: >> Am 04.04.2016 um 09:32 schrieb Alexander Graf: >>> The cache line flush helpers only work properly when they get aligned >>> start and end addresses. Round our flush range to cache line size. It's >>> safe because we're guaranteed to flush within a single page which has the >>> same cache attributes. >>> >>> Reported-by: Marek Vasut <marex@denx.de> >>> Signed-off-by: Alexander Graf <agraf@suse.de> >>> --- >>> lib/efi_loader/efi_runtime.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c >>> index 22bcd08..40acec0 100644 >>> --- a/lib/efi_loader/efi_runtime.c >>> +++ b/lib/efi_loader/efi_runtime.c >>> @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) >>> #endif >>> >>> *p = newaddr; >>> - flush_dcache_range((ulong)p, (ulong)&p[1]); >>> + flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1), >>> + ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE)); >> >> dragonboard410c_defconfig fails to build with this due to undefined >> CONFIG_SYS_CACHELINE_SIZE. Do we need to #ifdef here or is the >> dragonboard410c at fault for not defining it? > > Some USB drivers use it unconditionally, but I guess the dragonboard401c > doesn't enable USB. > > Let me come up with a more compatible version. This version is fine, dragonboard needs fixing. Every single board must define CONFIG_SYS_CACHELINE_SIZE .
On Mon, Apr 11, 2016 at 12:36:33AM +0200, Marek Vasut wrote: > On 04/11/2016 12:31 AM, Alexander Graf wrote: > > > > > > On 11.04.16 00:24, Andreas Färber wrote: > >> Am 04.04.2016 um 09:32 schrieb Alexander Graf: > >>> The cache line flush helpers only work properly when they get aligned > >>> start and end addresses. Round our flush range to cache line size. It's > >>> safe because we're guaranteed to flush within a single page which has the > >>> same cache attributes. > >>> > >>> Reported-by: Marek Vasut <marex@denx.de> > >>> Signed-off-by: Alexander Graf <agraf@suse.de> > >>> --- > >>> lib/efi_loader/efi_runtime.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c > >>> index 22bcd08..40acec0 100644 > >>> --- a/lib/efi_loader/efi_runtime.c > >>> +++ b/lib/efi_loader/efi_runtime.c > >>> @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) > >>> #endif > >>> > >>> *p = newaddr; > >>> - flush_dcache_range((ulong)p, (ulong)&p[1]); > >>> + flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1), > >>> + ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE)); > >> > >> dragonboard410c_defconfig fails to build with this due to undefined > >> CONFIG_SYS_CACHELINE_SIZE. Do we need to #ifdef here or is the > >> dragonboard410c at fault for not defining it? > > > > Some USB drivers use it unconditionally, but I guess the dragonboard401c > > doesn't enable USB. > > > > Let me come up with a more compatible version. > > This version is fine, dragonboard needs fixing. Every single board must > define CONFIG_SYS_CACHELINE_SIZE . Yeah... but this isn't quite the right time to break all of: arm: + mx28evk integratorap_cm720t at91sam9261ek_dataflash_cs3 at91sam9261ek_dataflash_cs0 integratorap_cm946es mx28evk_nand integratorcp_cm1136 at91sam9x5ek_nandflash at91sam9xeek_dataflash_cs1 at91sam9260ek_dataflash_cs0 at91sam9260ek_dataflash_cs1 at91sam9g20ek_nandflash meesc_dataflash at91sam9260ek_nandflash mx35pdk xfi3 at91sam9263ek_dataflash_cs0 at91sam9xeek_dataflash_cs0 at91sam9xeek_nandflash bg0900 highbank sansa_fuze_plus mx25pdk mx23evk at91sam9x5ek_mmc igep0030_nand m28evk at91sam9g20ek_dataflash_cs1 at91sam9g20ek_dataflash_cs0 integratorcp_cm946es at91sam9n12ek_nandflash at91sam9n12ek_spiflash da850_am18xxevm devkit3250 at91rm9200ek_ram picosam9g45 at91sam9n12ek_mmc integratorcp_cm926ejs integratorap_cm920t at91sam9m10g45ek_nandflash sc_sps_1 omapl138_lcdk mx28evk_spi integratorap_cm926ejs at91sam9rlek_mmc apx4devkit mx23_olinuxino at91sam9x5ek_spiflash at91sam9g10ek_nandflash at91sam9g10ek_dataflash_cs3 at91sam9g10ek_dataflash_cs0 at91sam9g20ek_2mmc_nandflash apf27 stv0991 at91sam9261ek_nandflash at91rm9200ek at91sam9g20ek_2mmc da850evm_direct_nor at91sam9263ek_norflash meesc at91sam9263ek_dataflash at91sam9m10g45ek_mmc armadillo-800eva at91sam9x5ek_dataflash da850evm usb_a9263_dataflash at91sam9rlek_nandflash mx28evk_auart_console at91sam9263ek_norflash_boot integratorcp_cm920t at91sam9263ek_nandflash at91sam9rlek_dataflash (and maybe a few more, toolchain here craps out due to libgcc problems), and we've talked about this before too I think. The right long term answer is to borrow the logic of arch/arm/mm/Kconfig and asm/cache.h from the kernel to get the right size, always. For the short term, we'll just work-around it.
On 04/12/2016 02:39 AM, Tom Rini wrote: > On Mon, Apr 11, 2016 at 12:36:33AM +0200, Marek Vasut wrote: > >> On 04/11/2016 12:31 AM, Alexander Graf wrote: >>> >>> >>> On 11.04.16 00:24, Andreas Färber wrote: >>>> Am 04.04.2016 um 09:32 schrieb Alexander Graf: >>>>> The cache line flush helpers only work properly when they get aligned >>>>> start and end addresses. Round our flush range to cache line size. It's >>>>> safe because we're guaranteed to flush within a single page which has the >>>>> same cache attributes. >>>>> >>>>> Reported-by: Marek Vasut <marex@denx.de> >>>>> Signed-off-by: Alexander Graf <agraf@suse.de> >>>>> --- >>>>> lib/efi_loader/efi_runtime.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c >>>>> index 22bcd08..40acec0 100644 >>>>> --- a/lib/efi_loader/efi_runtime.c >>>>> +++ b/lib/efi_loader/efi_runtime.c >>>>> @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) >>>>> #endif >>>>> >>>>> *p = newaddr; >>>>> - flush_dcache_range((ulong)p, (ulong)&p[1]); >>>>> + flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1), >>>>> + ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE)); >>>> >>>> dragonboard410c_defconfig fails to build with this due to undefined >>>> CONFIG_SYS_CACHELINE_SIZE. Do we need to #ifdef here or is the >>>> dragonboard410c at fault for not defining it? >>> >>> Some USB drivers use it unconditionally, but I guess the dragonboard401c >>> doesn't enable USB. >>> >>> Let me come up with a more compatible version. >> >> This version is fine, dragonboard needs fixing. Every single board must >> define CONFIG_SYS_CACHELINE_SIZE . > > Yeah... but this isn't quite the right time to break all of: > arm: + mx28evk integratorap_cm720t at91sam9261ek_dataflash_cs3 > at91sam9261ek_dataflash_cs0 integratorap_cm946es mx28evk_nand > integratorcp_cm1136 at91sam9x5ek_nandflash > at91sam9xeek_dataflash_cs1 at91sam9260ek_dataflash_cs0 > at91sam9260ek_dataflash_cs1 at91sam9g20ek_nandflash > meesc_dataflash at91sam9260ek_nandflash mx35pdk xfi3 > at91sam9263ek_dataflash_cs0 at91sam9xeek_dataflash_cs0 > at91sam9xeek_nandflash bg0900 highbank sansa_fuze_plus mx25pdk > mx23evk at91sam9x5ek_mmc igep0030_nand m28evk > at91sam9g20ek_dataflash_cs1 at91sam9g20ek_dataflash_cs0 > integratorcp_cm946es at91sam9n12ek_nandflash > at91sam9n12ek_spiflash da850_am18xxevm devkit3250 > at91rm9200ek_ram picosam9g45 at91sam9n12ek_mmc > integratorcp_cm926ejs integratorap_cm920t > at91sam9m10g45ek_nandflash sc_sps_1 omapl138_lcdk mx28evk_spi > integratorap_cm926ejs at91sam9rlek_mmc apx4devkit mx23_olinuxino > at91sam9x5ek_spiflash at91sam9g10ek_nandflash > at91sam9g10ek_dataflash_cs3 at91sam9g10ek_dataflash_cs0 > at91sam9g20ek_2mmc_nandflash apf27 stv0991 > at91sam9261ek_nandflash at91rm9200ek at91sam9g20ek_2mmc > da850evm_direct_nor at91sam9263ek_norflash meesc > at91sam9263ek_dataflash at91sam9m10g45ek_mmc armadillo-800eva > at91sam9x5ek_dataflash da850evm usb_a9263_dataflash > at91sam9rlek_nandflash mx28evk_auart_console > at91sam9263ek_norflash_boot integratorcp_cm920t > at91sam9263ek_nandflash at91sam9rlek_dataflash > (and maybe a few more, toolchain here craps out due to libgcc problems), Ha, libgcc ... :-( > and we've talked about this before too I think. The right long term > answer is to borrow the logic of arch/arm/mm/Kconfig and asm/cache.h > from the kernel to get the right size, always. For the short term, > we'll just work-around it. I thought we fixed these cache issues already, but that was on v7 only, dang. Now it is v4/v5 which broke.
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 22bcd08..40acec0 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -194,7 +194,8 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) #endif *p = newaddr; - flush_dcache_range((ulong)p, (ulong)&p[1]); + flush_dcache_range((ulong)p & ~(CONFIG_SYS_CACHELINE_SIZE - 1), + ALIGN((ulong)&p[1], CONFIG_SYS_CACHELINE_SIZE)); } #ifndef IS_RELA
The cache line flush helpers only work properly when they get aligned start and end addresses. Round our flush range to cache line size. It's safe because we're guaranteed to flush within a single page which has the same cache attributes. Reported-by: Marek Vasut <marex@denx.de> Signed-off-by: Alexander Graf <agraf@suse.de> --- lib/efi_loader/efi_runtime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)