Failed to boot ARM64 boards for recent linux-next
Shawn Lin
shawn.lin at rock-chips.com
Tue Mar 20 03:06:02 PDT 2018
Hi Marc,
在 2018/3/20 17:56, Marc Zyngier 写道:
> On 20/03/18 09:32, Shawn Lin wrote:
>> Hi Marc,
>>
>> On 2018/3/20 17:01, Marc Zyngier wrote:
>>> Hi Shawn,
>>>
>>> On 20/03/18 08:48, Shawn Lin wrote:
>>>> Hi Marc,
>>>>
>>>> I was able to boot my RK3399 board with in linux-next-20180314,
>>>> but not today. My bisect robot shows me it was introduced by
>>>>
>>>> commit d6062a6d62c643a06c393745d032da3e6441d4bd
>>>> Author: Marc Zyngier <marc.zyngier at arm.com>
>>>> Date: Fri Mar 9 14:53:19 2018 +0000
>>>>
>>>> irqchip/gic-v3: Reset APgRn registers at boot time
>>>>
>>>> Booting a crash kernel while in an interrupt handler is likely
>>>> to leave the Active Priority Registers with some state that
>>>> is not relevant to the new kernel, and is likely to lead
>>>> to erratic behaviours such as interrupts not firing as their
>>>> priority is already active.
>>>>
>>>> As a sanity measure, wipe the APRs clean on startup. We make
>>>> sure to wipe both group 0 and 1 registers in order to avoid
>>>> any surprise.
>>>>
>>>>
>>>> The panic log is here:
>>>> https://paste.ubuntu.com/p/7WrJJDG6JQ/
>>>>
>>>> Is it a known issue or is there a coming patch for that?
>>>
>>> Interesting. No, that wasn't the intention, but I may have missed a key
>>> detail (group 0 access traps to EL3 if SCR_EL3.FIQ==1). Can you have a
>>> go at the following hack, just to narrow it down:
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>> index 5bb7bb22f1c1..f8ff43b1d4f8 100644
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>> @@ -570,16 +570,12 @@ static void gic_cpu_sys_reg_init(void)
>>> switch(val + 1) {
>>> case 8:
>>> case 7:
>>> - write_gicreg(0, ICC_AP0R3_EL1);
>>> write_gicreg(0, ICC_AP1R3_EL1);
>>> - write_gicreg(0, ICC_AP0R2_EL1);
>>> write_gicreg(0, ICC_AP1R2_EL1);
>>> case 6:
>>> - write_gicreg(0, ICC_AP0R1_EL1);
>>> write_gicreg(0, ICC_AP1R1_EL1);
>>> case 5:
>>> case 4:
>>> - write_gicreg(0, ICC_AP0R0_EL1);
>>> write_gicreg(0, ICC_AP1R0_EL1);
>>> }
>>>
>>> Let me know if that helps.
>>>
>>
>> It works for me. Thanks!
> OK. Would you mind testing a much more complete patch?
Hmm.. the more complete patch doesn't work for me.
>
>>From 192786590930c205461ea0a379d295a2d0375fc1 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier at arm.com>
> Date: Tue, 20 Mar 2018 09:46:42 +0000
> Subject: [PATCH] irqchip/gic-v3: Check availability of Group0 before resetting
> AP0Rn
>
> We now try to reset the Active Priority Registers at boot time,
> without checking if we actually have access to them. Bad move.
> If the secure side has set SCR_EL3.FIQ=1, we'll trap to EL3, and
> the firmware may not be please to get such an exception.
>
> Instead, let's use PMR to find out if its value gets affected by
> SCR_EL3.FIQ being set. Only if a readback from PMR shows the same
> value (or a higher priority) will we be able to write to AP0Rn.
>
> Fixes: d6062a6d62c6 ("irqchip/gic-v3: Reset APgRn registers at boot time")
> Reported-by: Shawn Lin <shawn.lin at rock-chips.com>
> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
> ---
> arch/arm/include/asm/arch_gicv3.h | 6 +-----
> arch/arm64/include/asm/arch_gicv3.h | 5 -----
> drivers/irqchip/irq-gic-v3.c | 25 ++++++++++++++++++++-----
> 3 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h
> index 27288bdbd840..0bd530702118 100644
> --- a/arch/arm/include/asm/arch_gicv3.h
> +++ b/arch/arm/include/asm/arch_gicv3.h
> @@ -137,6 +137,7 @@ static inline u64 read_ ## a64(void) \
> return val; \
> }
>
> +CPUIF_MAP(ICC_PMR, ICC_PMR_EL1)
> CPUIF_MAP(ICC_AP0R0, ICC_AP0R0_EL1)
> CPUIF_MAP(ICC_AP0R1, ICC_AP0R1_EL1)
> CPUIF_MAP(ICC_AP0R2, ICC_AP0R2_EL1)
> @@ -206,11 +207,6 @@ static inline u32 gic_read_iar(void)
> return irqstat;
> }
>
> -static inline void gic_write_pmr(u32 val)
> -{
> - write_sysreg(val, ICC_PMR);
> -}
> -
> static inline void gic_write_ctlr(u32 val)
> {
> write_sysreg(val, ICC_CTLR);
> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> index 9becba9ab392..e278f94df0c9 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -76,11 +76,6 @@ static inline u64 gic_read_iar_cavium_thunderx(void)
> return irqstat;
> }
>
> -static inline void gic_write_pmr(u32 val)
> -{
> - write_sysreg_s(val, SYS_ICC_PMR_EL1);
> -}
> -
> static inline void gic_write_ctlr(u32 val)
> {
> write_sysreg_s(val, SYS_ICC_CTLR_EL1);
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5bb7bb22f1c1..a3f7624f5b00 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -532,6 +532,7 @@ static void gic_cpu_sys_reg_init(void)
> int i, cpu = smp_processor_id();
> u64 mpidr = cpu_logical_map(cpu);
> u64 need_rss = MPIDR_RS(mpidr);
> + bool group0;
> u32 val;
>
> /*
> @@ -545,7 +546,11 @@ static void gic_cpu_sys_reg_init(void)
> pr_err("GIC: unable to set SRE (disabled at EL2), panic ahead\n");
>
> /* Set priority mask register */
> - gic_write_pmr(DEFAULT_PMR_VALUE);
> + write_gicreg(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
> +
> + /* Is Group0 available to us? */
> + val = read_gicreg(ICC_PMR_EL1);
> + group0 = (val <= DEFAULT_PMR_VALUE);
>
> /*
> * Some firmwares hand over to the kernel with the BPR changed from
> @@ -567,19 +572,29 @@ static void gic_cpu_sys_reg_init(void)
> val &= ICC_CTLR_EL1_PRI_BITS_MASK;
> val >>= ICC_CTLR_EL1_PRI_BITS_SHIFT;
>
> + if (group0) {
> + switch(val + 1) {
> + case 8:
> + case 7:
> + write_gicreg(0, ICC_AP0R3_EL1);
> + write_gicreg(0, ICC_AP0R2_EL1);
> + case 6:
> + write_gicreg(0, ICC_AP0R1_EL1);
> + case 5:
> + case 4:
> + write_gicreg(0, ICC_AP0R0_EL1);
> + }
> + }
> +
> switch(val + 1) {
> case 8:
> case 7:
> - write_gicreg(0, ICC_AP0R3_EL1);
> write_gicreg(0, ICC_AP1R3_EL1);
> - write_gicreg(0, ICC_AP0R2_EL1);
> write_gicreg(0, ICC_AP1R2_EL1);
> case 6:
> - write_gicreg(0, ICC_AP0R1_EL1);
> write_gicreg(0, ICC_AP1R1_EL1);
> case 5:
> case 4:
> - write_gicreg(0, ICC_AP0R0_EL1);
> write_gicreg(0, ICC_AP1R0_EL1);
> }
>
>
More information about the Linux-rockchip
mailing list