Enable arm_global_timer for Zynq brakes boot

[Sören Brinkmann]

Hi Daniel,

On Mon, Jul 29, 2013 at 02:51:49PM +0200, Daniel Lezcano wrote:
> On 07/23/2013 12:41 AM, Sören Brinkmann wrote:
> > Hi,
> > 
> > adding LKML and LAKML (which I forgot on the original email, sorry)
> > 
> > On Mon, Jul 22, 2013 at 01:13:48PM -0700, Sören Brinkmann wrote:
> >> On Mon, Jul 22, 2013 at 12:54:26PM -0700, Stephen Boyd wrote:
> >>> On 07/22/13 11:25, Sören Brinkmann wrote:
> >>>> On Wed, Jul 17, 2013 at 06:22:35PM -0700, Stephen Boyd wrote:
> >>>>> On 07/17/13 17:59, Sören Brinkmann wrote:
> >>>>>> Hi Stephen,
> >>>>>>
> >>>>>> On Wed, Jul 17, 2013 at 04:50:34PM -0700, Stephen Boyd wrote:
> >>>>>>> On 07/17/13 14:04, Sören Brinkmann wrote:
> >>>>>>>> Hi all,
> >>>>>>>>
> >>>>>>>> I'm trying to enable the arm_global_timer on Zynq platforms with the
> >>>>>>>> attached patch. Unfortunately that patch breaks booting up. It hangs
> >>>>>>>> when handing over to init/early userspace (see attached boot.log).
> >>>>>>>>
> >>>>>>>> The funny thing is, if I remove either the global timer or the
> >>>>>>>> arm,cortex-a9-twd-timer node from my dts, it works. So, it looks like
> >>>>>>>> the two timer (drivers) interfere somehow. Does anybody have an idea of
> >>>>>>>> what is going on and probably even how to resolve it?
> >>>>>>>>
> >>>>>>>> The patch is based on commit c0d15cc in Linus' tree.
> >>>>>>> If you boot with one CPU does it hang? It looks like secondary CPUs
> >>>>>>> aren't getting interrupts but I'm not sure why. Maybe you can try this
> >>>>>>> patch and/or put some prints in the timer interrupt handler to see if
> >>>>>>> interrupts are firing on secondary CPUs.
> >>>>>> Your proposed patch does not seem to make a difference, but adding
> >>>>>> 'maxcpus=1' to the kernel command line makes the system boot.
> >>>>> Hmm I guess that doesn't really confirm much because TWD doesn't
> >>>>> register itself on maxcpus=1 systems, so it's the same as removing the
> >>>>> node from DT. Probably easier to put a printk in the interrupt handler
> >>>>> and confirm that you're receiving interrupts on secondary CPUs.
> >>>> Turns out it does work when I disable Zynq's cpuidle driver. I think I
> >>>> can blame that driver.
> >>>>
> >>>
> >>> Hmm.. Perhaps the arm_global_timer driver also needs FEAT_C3_STOP added
> >>> to it. Do you know if that timer is reset during low power modes?
> >>
> >> Our cpudidle driver is not powering off anything, AFAIK. I think it just
> >> executes 'wfi' on the CPU. I don't know how the timer core handles it,
> >> but I'd expect the CPU should find the timer just a it was left before
> >> entering idle (well, the timer continues to run I assume, but other than
> >> that).
> >> I'll do some debugging and see if I can find out what exactly causes the
> >> hang.
> > 
> > So, what I found:
> > The Zynq cpuidle driver provides two idle states, which are both
> > basically just ARM wfi states. But the second one set's these flags:
> > 	.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TIMER_STOP,
> > 
> > I don't know what these flags cause in detail. But the
> > CPUIDLE_FLAG_TIMER_STOP seemed suspicious, since wfi does not have any
> > effect on the timer. So, I removed that one and things are working.
> > 
> > I also tried the other approach: Leaving cpuidle as is and adding the
> > C3STOP flag to the global timer. That solves it too.
> > 
> > Does anybody know what the correct solution is?
> > In case the C3STOP flag is considered to be corret for the timer, I
> > could prepare a patch for that and bundle it with the one to enable the
> > timer for Zynq?
> 
> Hi Soren,
> 
> the CPUIDLE_FLAG_TIMER_STOP flag tells the cpuidle framework the local
> timer will be stopped when entering to the idle state. In this case, the
> cpuidle framework will call clockevents_notify(ENTER) and switches to a
> broadcast timer and will call clockevents_notify(EXIT) when exiting the
> idle state, switching the local timer back in use.
> 
> The C3STOP flag has a similar semantic than the CPUIDLE_FLAG_TIMER_STOP,
> that is the timer can be shutdown with a specific idle state. This flag
> is used by the tick broadcast code.
> 
> If the C3STOP flag is not set for a local timer, the
> CPUIDLE_FLAG_TIMER_STOP does not make sense because it will be ignored
> by the tick-broadcast code.
> 
> If the local timer could be shutdown at idle time, you *must* specify
> this flag.
> 
> If the idle state shutdowns the cpu with its local timer, you *must*
> specify the CPUIDLE_FLAG_TIMER_STOP flag for this specific state.
> 
> At the first glance, the idle state #2 is aimed to do DDR self refresh
> and to switch to WFI, so no power gating, then no local timer down. The
> CPUIDLE_FLAG_TIMER_STOP shouldn't be used here.
> 
> IIUC, the global timer does not belong to the CPU and the cluster power
> domains, so it can't be shutdown: the C3STOP shouldn't be used.
> 
> I hope that helps.

Thanks for the explanation. I have to discuss with Michal what to do about the
cpuidle driver and then we should be good to use the global timer.

	Thanks,
	Sören