[PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

On 09/16/2014 06:42 AM, Catalin Marinas wrote:

On Mon, Sep 15, 2014 at 11:51:14PM +0100, Christopher Covington wrote:

quoted

Hi Sonny,

On 09/15/2014 06:04 PM, Sonny Rao wrote:

quoted

On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao [off-list ref] wrote:

quoted

On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd [off-list ref] wrote:

quoted

On 09/15/14 14:47, Sonny Rao wrote:

quoted

On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd [off-list ref] wrote:

quoted

On 09/15/14 04:10, Catalin Marinas wrote:

quoted

On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:

quoted

On 09/12/14 05:14, Marc Zyngier wrote:

quoted

We surely can handle the UNDEF and do something there. We just can't do
it the way Doug described it above.

I suggested doing that for something else a while ago and Will and Dave
we're not thrilled[1]. The suggestion back then was to use DT to
indicate what mode the kernel is running in.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html

I think the context was slightly different. As I re-read the thread, it
seems that the discussion was around whether to use some SMC interface
or not based on whether the kernel is running secure or non-secure. The
argument made by Will was to actually specify the type of the firmware
SMC interface in the DT and use it in the kernel (and probably assume
the kernel is running in secure mode if no smc interface is specified in
the DT; you could have both though, running in secure mode and also
having firmware).

In this arch timer case, we need to work around a firmware bug (or
feature as 32-bit ARM kernels never required CNTVOFF initialisation by
firmware, no matter how small such firmware is). We don't expect a
specific SMC call to initialise CNTVOFF, so we can't describe it in the
DT.

Agreed, we can't described SMC calls that don't exist. From my
perspective it's just another part of the cpu boot sequence that needs
to be handled in the kernel, so describing the requirement via the
cpu-boot method seems appropriate. It seems like we're making it harder
than it should be by handling the undef when we could have slightly
different SMP boot code (and suspend/resume code) depending on the boot
method property.

+heiko

So, for the case of rk3288, based on this discussion what I'm going to
propose is to add code to rockchip.c which looks for a particular SMP
enable method -- say something like "rockchip,rk3288-smp-secure-svc"
which will then assume we have been booted in secure SVC mode and do
the CNTVOFF fixup.  I believe, it will need to do this on the boot CPU
as well, so I think it will need to scan the DT fairly early on the
boot CPU and also perform the function there.

I'll look into implementing this and post code.  Comments and
suggestions appreciated, thanks.

What goes wrong if we read the cntvoff from the boot CPU during
smp_prepare_cpus() phase and use that to set the cntvoff on the other
CPUs? That avoids needing to do anything very early by making the value
the same. It does mean that cntvoff is some random out of reset value
for CPU0, but at least it's consistent.

I think we cannot read the value if we're not in hyp mode.

Well, thinking about it a little more, I think you still have a good point.

We don't need to do this early on, as long as we haven't started using
the arch timers yet.  If we are still able to do this at the point
where we're executing the code in arch/arm/mach-rockchip/platsmp.c
that finds the enable method then we can just handle it there.

I've been playing around with the probe-based approach and while I need to do
a lot more testing, it seems to be working for the first tens of instructions.
I hope to be able to share a draft of that soon. Basically, I just read the
current NSACR value and write it back (although maybe in the long term we
would want to make sure a few of those bits are set or cleared). If that
succeeds, we know we're in secure SVC and can proceed to set up MON and HYP.

But when it doesn't succeed, you get an undefined instruction fault
(since NSACR is only writable in secure mode).

Yes. I see it as a conditional branch to VBAR+4 with a mode switch side effect.

Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.