On Tue, 2012-05-15 at 14:44 -0700, Kevin Hilman wrote:
> Santosh,
>
> Tero Kristo <t-kristo@xxxxxx> writes:
>
> > From: Santosh Shilimkar <santosh.shilimkar@xxxxxx>
> >
> > GIC distributor control register has changed between CortexA9 r1pX and
> > r2pX. The Control Register secure banked version is now composed of 2
> > bits:
> > bit 0 == Secure Enable
> > bit 1 == Non-Secure Enable
> > The Non-Secure banked register has not changed.
>
> For those without the r1pX TRM handy, please include what this look like
> before (presumably 1 bit?) The changelog and in-code comments should
> both be enhanced.
>
> > Since the ROM Code is based on the r1pX GIC, the CPU1 GIC restoration
> > will cause a problem to CPU0 Non-Secure SW.
>
> Please describe the problem, so we can better understand the specifics
> of the workaround.
Santosh, can you provide a better changelog for this?
>
> Also, is there an erratum number for this?
Nothing public. I don't think there are any public documents about ROM
code, including erratas.
>
> > The workaround must be:
> > 1) Before doing the CPU1 wakeup, CPU0 must disable
> > the GIC distributor
> > 2) CPU1 must re-enable the GIC distributor on
> > it's wakeup path.
>
> Again, because the problem was not described, I am not entirely sure why
> the workaround "must" be this, and how it fixes the problem. Remember
> to put yourself in the shoes of a reviewer who has not been deeply
> involved in the code, so what seems obvious to you will not be obvious
> to the reviewer without having to study the code and dig in to the TRMs.
>
> > With this procedure, the GIC configuration done between the
> > CPU0 wakeup and CPU1 wakeup will not be lost but during this
> > short windows, the CPU0 will not receive interrupts
>
> Hmm, so I guess this means that CPU0 always has to wakeup first, even on GP
> devices?
Yes.
>
> Also, the changelog doesn't describe what power states this affects, and
> whether it's an idle problem or just a supend problem. A quick glance
> at the code suggests that only suspend is being addressed by this patch.
> However, with the addition of coupled CPUidle (coming very soon now),
> won't we have this same problem in idle? IMO, this patch should address
> both.
My understanding is that this happens if mpu_pwrdm goes off. Cpuidle
support is not added to this patch as it does not exist yet. Probably
shouldn't be that big fix.
>
> This workaround also assumes that you always want CPU1 to wakeup
> immediately after CPU0. I guess that will be the case for the coupled
> states that would be affected by this bug, but the changelog should
> describe that as well
Yes, this patch assumes both CPUs attempt to wake-up, thus cpu1 is stuck
in the hold loop. The device-off set patch #17 addresses this issue and
puts cpu1 back to idle immediately if no wake request is received from
cpu0. That patch should be possible to move to this set if needed.
>
> Some minor comments below...
>
> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@xxxxxx>
> > Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
> > ---
> > arch/arm/mach-omap2/common.h | 2 +
> > arch/arm/mach-omap2/omap-headsmp.S | 36 +++++++++++++++++++++++++++++
> > arch/arm/mach-omap2/omap-mpuss-lowpower.c | 9 ++++++-
> > arch/arm/mach-omap2/omap-smp.c | 28 +++++++++++++++++++++-
> > arch/arm/mach-omap2/omap4-common.c | 8 +++++-
> > 5 files changed, 80 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
> > index 57da7f4..48d1ebe 100644
> > --- a/arch/arm/mach-omap2/common.h
> > +++ b/arch/arm/mach-omap2/common.h
> > @@ -199,6 +199,7 @@ static inline void __iomem *omap4_get_scu_base(void)
> > #endif
> >
> > extern void __init gic_init_irq(void);
> > +extern void gic_dist_disable(void);
> > extern void omap_smc1(u32 fn, u32 arg);
> > extern void __iomem *omap4_get_sar_ram_base(void);
> > extern void omap_do_wfi(void);
> > @@ -206,6 +207,7 @@ extern void omap_do_wfi(void);
> > #ifdef CONFIG_SMP
> > /* Needed for secondary core boot */
> > extern void omap_secondary_startup(void);
> > +extern void omap_secondary_startup_4460(void);
> > extern u32 omap_modify_auxcoreboot0(u32 set_mask, u32 clear_mask);
> > extern void omap_auxcoreboot_addr(u32 cpu_addr);
> > extern u32 omap_read_auxcoreboot0(void);
> > diff --git a/arch/arm/mach-omap2/omap-headsmp.S b/arch/arm/mach-omap2/omap-headsmp.S
> > index 503ac77..d602555 100644
> > --- a/arch/arm/mach-omap2/omap-headsmp.S
> > +++ b/arch/arm/mach-omap2/omap-headsmp.S
> > @@ -43,3 +43,39 @@ hold: ldr r12,=0x103
> > b secondary_startup
> > ENDPROC(omap_secondary_startup)
> >
> > +ENTRY(omap_secondary_startup_4460)
> > +hold_2: ldr r12,=0x103
> > + dsb
> > + smc #0 @ read from AuxCoreBoot0
> > + mov r0, r0, lsr #9
> > + mrc p15, 0, r4, c0, c0, 5
> > + and r4, r4, #0x0f
> > + cmp r0, r4
> > + bne hold_2
> > +
> > + /*
> > + * GIC distributor control register has changed between
> > + * CortexA9 r1pX and r2pX. The Control Register secure
> > + * banked version is now composed of 2 bits:
> > + * bit 0 == Secure Enable
> > + * bit 1 == Non-Secure Enable
> > + * The Non-Secure banked register has not changed
> > + * Because the ROM Code is based on the r1pX GIC, the CPU1
> > + * GIC restoration will cause a problem to CPU0 Non-Secure SW.
> > + * The workaround must be:
> > + * 1) Before doing the CPU1 wakeup, CPU0 must disable
> > + * the GIC distributor
> > + * 2) CPU1 must re-enable the GIC distributor on
> > + * it's wakeup path.
> > + */
> > + ldr r1, =0x48241000
>
> Why not OMAP44XX_GIC_DIST_BASE for readability sake?
Will change this.
>
> > + ldr r0, [r1]
> > + orr r0, #1
> > + str r0, [r1]
> > +
> > + /*
> > + * we've been released from the wait loop,secondary_stack
> > + * should now contain the SVC stack for this core
> > + */
> > + b secondary_startup
> > +ENDPROC(omap_secondary_startup_4460)
> > diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> > index 13670aa..e02c082 100644
> > --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> > +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> > @@ -68,6 +68,7 @@ struct omap4_cpu_pm_info {
> > void __iomem *scu_sar_addr;
> > void __iomem *wkup_sar_addr;
> > void __iomem *l2x0_sar_addr;
> > + void (*secondary_startup)(void);
> > };
> >
> > static DEFINE_PER_CPU(struct omap4_cpu_pm_info, omap4_pm_info);
> > @@ -300,6 +301,7 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
> > int __cpuinit omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state)
> > {
> > unsigned int cpu_state = 0;
> > + struct omap4_cpu_pm_info *pm_info = &per_cpu(omap4_pm_info, cpu);
> >
> > if (omap_rev() == OMAP4430_REV_ES1_0)
> > return -ENXIO;
> > @@ -309,7 +311,7 @@ int __cpuinit omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state)
> >
> > clear_cpu_prev_pwrst(cpu);
> > set_cpu_next_pwrst(cpu, power_state);
> > - set_cpu_wakeup_addr(cpu, virt_to_phys(omap_secondary_startup));
> > + set_cpu_wakeup_addr(cpu, virt_to_phys(pm_info->secondary_startup));
> > scu_pwrst_prepare(cpu, power_state);
> >
> > /*
> > @@ -360,6 +362,11 @@ int __init omap4_mpuss_init(void)
> > pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
> > pm_info->wkup_sar_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
> > pm_info->l2x0_sar_addr = sar_base + L2X0_SAVE_OFFSET1;
> > + if (cpu_is_omap446x())
> > + pm_info->secondary_startup = omap_secondary_startup_4460;
> > + else
> > + pm_info->secondary_startup = omap_secondary_startup;
>
> Does this exist on 4470 too?
No, only valid for 4460. ROM code should be fixed on 4470 (haven't
tested that though as I don't have access to 4470 devices.)
>
> Using a PM erratum check here instead of cpu_is* would make this more
> scalable.
>
> Same comment for the cpu_is* check in wakeup_secondary()
>
> Then, where the erratum is defined, it should state clearly why this
> affects 446x and not before (presumably because 4430 has r1pX and 4460
> has r2pX.)
Yea, I can add errata flag for this. The errata support for omap4 PM is
currently defined in the device off set, so I guess I need to pull that
to this set.
>
> > pm_info->pwrdm = pwrdm_lookup("cpu1_pwrdm");
> > if (!pm_info->pwrdm) {
> > pr_err("Lookup failed for CPU1 pwrdm\n");
> > diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
> > index deffbf1..c3eb9e8 100644
> > --- a/arch/arm/mach-omap2/omap-smp.c
> > +++ b/arch/arm/mach-omap2/omap-smp.c
> > @@ -33,6 +33,7 @@
> >
> > /* SCU base address */
> > static void __iomem *scu_base;
> > +bool omap4_smp_romcode_errata;
>
> static?
Yes should be.
-Tero
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
