From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ziyuan Xu Date: Mon, 1 Aug 2016 09:07:05 +0800 Subject: [U-Boot] [PATCH] arm: re-implement proper ISB instruction for ARMv7-A In-Reply-To: <20160731142707.GE17970@bill-the-cat> References: <1469700807-22212-1-git-send-email-xzy.xu@rock-chips.com> <20160728221550.GP5926@bill-the-cat> <579A9671.7090806@rock-chips.com> <20160729003437.GR5926@bill-the-cat> <579AAC15.2020804@rock-chips.com> <20160729011215.GS5926@bill-the-cat> <56743f5c-9f99-eb43-6a62-883e00253a11@rock-chips.com> <20160731142707.GE17970@bill-the-cat> Message-ID: <3c96c6f3-8285-933d-494a-e35beca501a5@rock-chips.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 2016?07?31? 22:27, Tom Rini wrote: > On Sun, Jul 31, 2016 at 11:59:19AM +0800, Ziyuan Xu wrote: >> Hi Tom, >> >> >> On 2016?07?29? 09:12, Tom Rini wrote: >>> On Fri, Jul 29, 2016 at 09:06:29AM +0800, Ziyuan Xu wrote: >>>> Hi Tom, >>>> >>>> On 2016?07?29? 08:34, Tom Rini wrote: >>>>> On Fri, Jul 29, 2016 at 07:34:09AM +0800, Ziyuan Xu wrote: >>>>>> Hi Tom, >>>>>> >>>>>> On 2016?07?29? 06:15, Tom Rini wrote: >>>>>>> On Thu, Jul 28, 2016 at 07:03:17PM +0800, Chen-Yu Tsai wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On Thu, Jul 28, 2016 at 6:13 PM, Ziyuan Xu wrote: >>>>>>>>> For ARMv7-A architecture, the valid ISB instruction is asm volatile("isb"). >>>>>>>>> >>>>>>>>> This patch fixes the U-Boot was stuck in invalidate_dcache_all() before >>>>>>>>> booting linux kernel, which occurred on rk3288-base development board >>>>>>>>> such as evb-rk3288, rock2-rk3288. And something output via console like: >>>>>>>>> >>>>>>>>> => bootz 0x2000000 >>>>>>>>> 0x02000000 >>>>>>>>> ramdisk start = 0x00000000, ramdisk end = 0x00000000 >>>>>>>>> Continuing to boot without FDT >>>>>>>>> Initial value for argc=3 >>>>>>>>> Final value for argc=3 >>>>>>>>> using: ATAGS >>>>>>>>> >>>>>>>>> Starting kernel ... >>>>>>>>> >>>>>>>>> Linux kernel exactly the same way(see arch/arm/include/asm/barrier.h). >>>>>>>>> >>>>>>>>> Signed-off-by: Ziyuan Xu >>>>>>>>> --- >>>>>>>>> >>>>>>>>> arch/arm/include/asm/system.h | 6 ++++-- >>>>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h >>>>>>>>> index 2bdc0be..12d4ba0 100644 >>>>>>>>> --- a/arch/arm/include/asm/system.h >>>>>>>>> +++ b/arch/arm/include/asm/system.h >>>>>>>>> @@ -227,13 +227,15 @@ void __noreturn psci_system_reset(bool smc); >>>>>>>>> */ >>>>>>>>> void save_boot_params_ret(void); >>>>>>>>> >>>>>>>>> -#define isb() __asm__ __volatile__ ("" : : : "memory") >>>>>>>>> - >>>>>>>>> #define nop() __asm__ __volatile__("mov\tr0,r0\t@ nop\n\t"); >>>>>>>>> >>>>>>>>> #ifdef __ARM_ARCH_7A__ >>>>>>>>> +#define isb() __asm__ __volatile__ ("isb" : : : "memory") >>>>>>>>> + >>>>>>>>> #define wfi() __asm__ __volatile__ ("wfi" : : : "memory") >>>>>>>>> #else >>>>>>>>> +#define isb() __asm__ __volatile__ ("" : : : "memory") >>>>>>>>> + >>>>>>>>> #define wfi() >>>>>>>>> #endif >>>>>>>>> >>>>>>>> arch/arm/include/asm/barriers.h already has a proper set of >>>>>>>> ISB/DSB macros. Please consider using those instead. >>>>>>> Please fix arch/arm/include/asm/system.h to use the macros found in >>>>>>> barriers.h rather than have their own versions. Thanks! >>>>>> If I understand correctly, I can change into as bellow: >>>>>> #define isb() ISB >>>>>> How about it? >>>>> Well, I'd rather not have ISB and isb, just ISB, which means we might >>>>> have to fix other places too. If that starts looking too huge, we can >>>>> do this in steps and just do what you suggested for now and for next >>>>> release move everything over. >>>> As I mentioned before, arch/arm/include/asm/barriers.h defined ISB >>>> macro. If I only want to fix the issue which I hit on rk3288 board, >>>> I just use ISB in arch/arm/include/asm/system.h::set_cr() instead of >>>> isb(). But isb() had been invoked in some places. >>>> >>>> I can't verify integrallty after all revision, it involve some >>>> boards and feature. But this does fix for rk3288, if you agree with >>>> me, could you apply it provisionally?:-) >>> I would really like to try and fix the other possibly latent issues that >>> we have by not calling a real ISB. Please try moving towards all places >>> that need an isb calling the correct one from barriers.h and giving it a >>> spin on the hardware you have available. >> I used ISB instead of isb(), everything works sane on my rockchip >> rk3288 boards.:-) >> Will you send a patch to fix it and other possibly latent issues? Or >> apply this temporarily? > Please send a patch of all of the changes you did, thanks! Tom, it's impracticable only use ISB for all situation instead of isb(). There're two isb() invocation in drivers/ddr/fsl/fsl_ddr_gen4.c, and not only used on ARM architecture board, but also powerpc. See below: arch/powerpc/include/asm/config_mpc85xx.h:769:#define CONFIG_SYS_FSL_DDRC_GEN4 arch/powerpc/include/asm/config_mpc85xx.h:822:#define CONFIG_SYS_FSL_DDRC_GEN4 arch/powerpc/include/asm/config_mpc85xx.h:955: !defined(CONFIG_SYS_FSL_DDRC_GEN4) arch/arm/include/asm/arch-ls102xa/config.h:100:#define CONFIG_SYS_FSL_DDRC_GEN4 arch/arm/include/asm/arch-fsl-layerscape/config.h:13:#define CONFIG_SYS_FSL_DDRC_GEN4 If you insist on ISB only, we have to differentiate arch via CONFIG_SYS_ARCH, seems twisted. How about it? #define isb() ISB >