Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Tue, 10 Jun 2014, Catalin Marinas wrote:
> On Tue, Jun 10, 2014 at 05:49:01PM +0100, Nicolas Pitre wrote:
> > The M-class processor should be treated the same way as firmware.  It 
> > ought to be flexible (certainly more than hardwired hardware), but it 
> > shares all the same downsides as firmware and the same concerns apply.
> 
> Yes, we can treat it as firmware, but we don't have a better alternative
> to move the functionality into the kernel (well, we could at least allow
> the kernel to load a binary blob and restart the controller).

That would address the "easy to update in the field" side of the story.  
So far I've not seen this aspect been addressed with a serious plan 
anywhere.

> > The reaction from the hardware people often is "the software is crap and 
> > makes our hardware look bad, we know better, so let's make it easier on 
> > those poor software dudes by handling power management in hardware 
> > instead".  But ultimately the hardware just can't predict things like 
> > software can.  It might do a better job than the current software state 
> > of affairs, but most likely not be as efficient as a proper software 
> > architecture.  The hardware may only be reactive, whereas the software 
> > can be proactive (when properly done that is).
> 
> Indeed. But that's not the aim of the power controller on our boards.
> It's just a mechanism for safely changing sleep states, CPU frequencies
> but entirely under the OS decision.

Sure.  But then you might want to consider that some usage scenarios 
might benefit from the ability to abort a request, or monitor the 
progress of a request for software timing purposes, or accept parallel 
requests rather than serialize them, etc.  Given the flexibility to 
extend beyond a rigid interface, the system may become even more 
efficient overall, albeit with added complexity in the implementation. 
But for that to work it has to be cheaply achievable.

> > I sense from your paragraph above that ARM might be going the same 
> > direction as X86 and that would be very sad.  Maybe the best compromise 
> > would be for all knobs to be made available to software if software 
> > wants to turn off the hardware auto-pilot and take control.  This way 
> > the hardware guys would cover their arses while still allowing for the 
> > possibility that software might be able to out perform the hardware 
> > solution.
> 
> I'm definitely not suggesting ARM is going the same route. Just trying
> to show that ARM is slightly better here.
> 
> As a personal opinion, I like the simplicity of writing a register to
> change the P-state but I don't like the non-determinism of the x86
> hardware w.r.t. CPU performance. There are however some "policy" aspects
> which I find interesting (like detecting whether the workload is memory
> bound and automatically lowering the CPU frequency; the OS cannot react
> this fast).

This is not really a policy.  This is a straight-forward waterfall 
dependency.  There is simply nothing you can do with those CPU clock 
cycles when stalled most of the time waiting for memory queries to come 
back so the choice is obvious.  If however this has a significant impact 
on code execution speed then this becomes a tradeoff and the choice to 
use this feature or not (the policy) must be implemented in software.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Catalin Marinas]

On Tue, Jun 10, 2014 at 05:49:01PM +0100, Nicolas Pitre wrote:
> On Tue, 10 Jun 2014, Catalin Marinas wrote:
> > On Tue, Jun 10, 2014 at 12:25:47AM -0400, Nicolas Pitre wrote:
> > > On Mon, 9 Jun 2014, Lorenzo Pieralisi wrote:
> > > > 4) When I am talking about firmware I am talking about sequences that
> > > >are very close to HW (disabling C bit, cleaning caches, exiting
> > > >coherency). Erratas notwithstanding, they are being standardized at
> > > >ARM the best we can. They might even end up being implemented in HW
> > > >in the not so far future. I understand they are tricky, I understand
> > > >they take lots of time to implement them and to debug them, what I
> > > >want to say is that they are becoming standard and we _must_ reuse 
> > > > the
> > > >same code for all ARM platforms. You can implement them in MCPM (see
> > > >(1)) or in firmware (and please do not start painting me as firmware
> > > >hugger here, I am referring to standard power down sequences that
> > > >again, are very close to HW state machines 
> > > 
> > > That's where the disconnect lies.  On the one hand you say "I understand 
> > > they are tricky, I understand they take lots of time to implement them 
> > > and to debug them" and on the other hand you say "They might end up being 
> > > implemented in HW in the not so far future."  That simply makes no 
> > > economical sense at all!
> > 
> > It makes lots of sense, though not from a software maintainability
> > perspective. It would be nice if everything still looked like ARM7TDMI
> > but in the race for performance (vs power), hardware becomes more
> > complex and it's not just the CPU but adjacent parts like interconnects,
> > caches, asynchronous bridges, voltage shifters, memory controllers,
> > clocks/PLLs etc. Many of these are simply hidden from the high level OS
> > like Linux because the OS assumes certain configuration (e.g. access to
> > memory) and it's only the hardware itself that knows in what order they
> > can be turned on or off (when triggered explicitly by the OS or an
> > external event).
> 
> I agree when the hardware has to handle parallel dependencies ordered in 
> waterfall style. In such cases there is usually no point relying on 
> software to implement what is nevertheless simple determinism with 
> no possible alternative usage.
> 
> But the *most* important thing is what you put in parens, so let me 
> emphasize on what you just said:
> 
>   When triggered _explicitly_ by the OS or external events

I don't think anyone is arguing that the policy should not be in the OS.
But part of the mechanism can be in the OS and part in firmware, SCP or
hardware. The kernel part can be a simple PSCI call or more complex
setup (possibly MCPM-based) which usually ends up with a WFI. This WFI,
however, triggers further hardware changes which may be handled by
dedicated power controller.

> > Having an dedicated power controller (e.g. M-class
> > processor) to handle some of these is a rather flexible approach, other
> > bits require RTL (and usually impossible to update).
> 
> The M-class processor should be treated the same way as firmware.  It 
> ought to be flexible (certainly more than hardwired hardware), but it 
> shares all the same downsides as firmware and the same concerns apply.

Yes, we can treat it as firmware, but we don't have a better alternative
to move the functionality into the kernel (well, we could at least allow
the kernel to load a binary blob and restart the controller).

> > > When some operation is 1) tricky and takes time to debug, and 2) not 
> > > performance critical (no one is trying to get in and out of idle or 
> > > hibernation a billion times per second), then you should never ever put 
> > > such a thing in firmware, and hardware should be completely out of the 
> > > question!
> > 
> > I agree that things can go wrong (both in hardware and software, no
> > matter where it runs) but please don't think that such power
> > architecture has been specifically engineered to hide the hardware from
> > Linux. It's a necessity for complex systems and the optimal solution is
> > not always simplification (it's not just ARM+vendors doing this, just
> > look at the power model of modern x86 processors, hidden nicely from the
> > software behind a few registers while making things harder for scheduler
> > which cannot rely on a constant performance level; but it's a trade-off
> > they are happy to make).
> 
> I'll claim that this is a bad tradeoff.  And the reason why some 
> hardware architects might think it is a good one is because so far we 
> really sucked at software based power management in Linux (and possibly 
> other OSes as well).  Hence the (fairly recent) realization that power 
> management has to be integrated and under control of the scheduler 
> rather than existing as some ad hoc subsystem.

But even this is a complex problem. Anyway, I don't think the (ARM at
least) hardwa

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Tue, 10 Jun 2014, Catalin Marinas wrote:

> Hi Nico,
> 
> Sorry, I can't stay away from this thread ;)

;-)

> On Tue, Jun 10, 2014 at 12:25:47AM -0400, Nicolas Pitre wrote:
> > On Mon, 9 Jun 2014, Lorenzo Pieralisi wrote:
> > > 4) When I am talking about firmware I am talking about sequences that
> > >are very close to HW (disabling C bit, cleaning caches, exiting
> > >coherency). Erratas notwithstanding, they are being standardized at
> > >ARM the best we can. They might even end up being implemented in HW
> > >in the not so far future. I understand they are tricky, I understand
> > >they take lots of time to implement them and to debug them, what I
> > >want to say is that they are becoming standard and we _must_ reuse the
> > >same code for all ARM platforms. You can implement them in MCPM (see
> > >(1)) or in firmware (and please do not start painting me as firmware
> > >hugger here, I am referring to standard power down sequences that
> > >again, are very close to HW state machines 
> > 
> > That's where the disconnect lies.  On the one hand you say "I understand 
> > they are tricky, I understand they take lots of time to implement them 
> > and to debug them" and on the other hand you say "They might end up being 
> > implemented in HW in the not so far future."  That simply makes no 
> > economical sense at all!
> 
> It makes lots of sense, though not from a software maintainability
> perspective. It would be nice if everything still looked like ARM7TDMI
> but in the race for performance (vs power), hardware becomes more
> complex and it's not just the CPU but adjacent parts like interconnects,
> caches, asynchronous bridges, voltage shifters, memory controllers,
> clocks/PLLs etc. Many of these are simply hidden from the high level OS
> like Linux because the OS assumes certain configuration (e.g. access to
> memory) and it's only the hardware itself that knows in what order they
> can be turned on or off (when triggered explicitly by the OS or an
> external event).

I agree when the hardware has to handle parallel dependencies ordered in 
waterfall style. In such cases there is usually no point relying on 
software to implement what is nevertheless simple determinism with 
no possible alternative usage.

But the *most* important thing is what you put in parents, so let me 
emphasize on what you just said:

When triggered _explicitly_ by the OS or external events

> Having an dedicated power controller (e.g. M-class
> processor) to handle some of these is a rather flexible approach, other
> bits require RTL (and usually impossible to update).

The M-class processor should be treated the same way as firmware.  It 
ought to be flexible (certainly more than hardwired hardware), but it 
shares all the same downsides as firmware and the same concerns apply.

> > When some operation is 1) tricky and takes time to debug, and 2) not 
> > performance critical (no one is trying to get in and out of idle or 
> > hibernation a billion times per second), then you should never ever put 
> > such a thing in firmware, and hardware should be completely out of the 
> > question!
> 
> I agree that things can go wrong (both in hardware and software, no
> matter where it runs) but please don't think that such power
> architecture has been specifically engineered to hide the hardware from
> Linux. It's a necessity for complex systems and the optimal solution is
> not always simplification (it's not just ARM+vendors doing this, just
> look at the power model of modern x86 processors, hidden nicely from the
> software behind a few registers while making things harder for scheduler
> which cannot rely on a constant performance level; but it's a trade-off
> they are happy to make).

I'll claim that this is a bad tradeoff.  And the reason why some 
hardware architects might think it is a good one is because so far we 
really sucked at software based power management in Linux (and possibly 
other OSes as well).  Hence the (fairly recent) realization that power 
management has to be integrated and under control of the scheduler 
rather than existing as some ad hoc subsystem.

The reaction from the hardware people often is "the software is crap and 
makes our hardware look bad, we know better, so let's make it easier on 
those poor software dudes by handling power management in hardware 
instead".  But ultimately the hardware just can't predict things like 
software can.  It might do a better job than the current software state 
of affairs, but most likely not be as efficient as a proper software 
architecture.  The hardware may only be reactive, whereas the software 
can be proactive (when properly done that is).

I sense from your paragraph above that ARM might be going the same 
direction as X86 and that would be very sad.  Maybe the best compromise 
would be for all knobs to be made available to software if software 
wants to turn off the hardware auto-pilot and take cont

Re: Problems booting exynos5420 with >1 CPU

[Catalin Marinas]

Hi Nico,

Sorry, I can't stay away from this thread ;)

On Tue, Jun 10, 2014 at 12:25:47AM -0400, Nicolas Pitre wrote:
> On Mon, 9 Jun 2014, Lorenzo Pieralisi wrote:
> > 4) When I am talking about firmware I am talking about sequences that
> >are very close to HW (disabling C bit, cleaning caches, exiting
> >coherency). Erratas notwithstanding, they are being standardized at
> >ARM the best we can. They might even end up being implemented in HW
> >in the not so far future. I understand they are tricky, I understand
> >they take lots of time to implement them and to debug them, what I
> >want to say is that they are becoming standard and we _must_ reuse the
> >same code for all ARM platforms. You can implement them in MCPM (see
> >(1)) or in firmware (and please do not start painting me as firmware
> >hugger here, I am referring to standard power down sequences that
> >again, are very close to HW state machines 
> 
> That's where the disconnect lies.  On the one hand you say "I understand 
> they are tricky, I understand they take lots of time to implement them 
> and to debug them" and on the other hand you say "They might end up being 
> implemented in HW in the not so far future."  That simply makes no 
> economical sense at all!

It makes lots of sense, though not from a software maintainability
perspective. It would be nice if everything still looked like ARM7TDMI
but in the race for performance (vs power), hardware becomes more
complex and it's not just the CPU but adjacent parts like interconnects,
caches, asynchronous bridges, voltage shifters, memory controllers,
clocks/PLLs etc. Many of these are simply hidden from the high level OS
like Linux because the OS assumes certain configuration (e.g. access to
memory) and it's only the hardware itself that knows in what order they
can be turned on or off (when triggered explicitly by the OS or an
external event). Having an dedicated power controller (e.g. M-class
processor) to handle some of these is a rather flexible approach, other
bits require RTL (and usually impossible to update).

> When some operation is 1) tricky and takes time to debug, and 2) not 
> performance critical (no one is trying to get in and out of idle or 
> hibernation a billion times per second), then you should never ever put 
> such a thing in firmware, and hardware should be completely out of the 
> question!

I agree that things can go wrong (both in hardware and software, no
matter where it runs) but please don't think that such power
architecture has been specifically engineered to hide the hardware from
Linux. It's a necessity for complex systems and the optimal solution is
not always simplification (it's not just ARM+vendors doing this, just
look at the power model of modern x86 processors, hidden nicely from the
software behind a few registers while making things harder for scheduler
which cannot rely on a constant performance level; but it's a trade-off
they are happy to make).

> >and more importantly if they
> >HAVE to run in secure world that's the only solution we have unless you
> >want to split race conditions between kernel and secure world).
> 
> If they HAVE to run in secure world then your secure world architecture 
> is simply misdesigned, period.  Someone must have ignored the economics 
> of modern software development to have come up with this.

That's the trade-off between software complexity and hardware cost,
gates, power consumption. You can do proper physical separation of the
secure services but this would require a separate CPU that is rarely
used and adds to the overall SoC cost. On large scale hardware
deployment, it's exactly economics that matter and these translate into
hardware cost. The software cost is irrelevant here, whether we like it
or not.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Lorenzo Pieralisi]

On Tue, Jun 10, 2014 at 05:25:47AM +0100, Nicolas Pitre wrote:
> On Mon, 9 Jun 2014, Lorenzo Pieralisi wrote:
> 
> > I commented on Nico's patch because I did not like how it was
> > implemented (at least remove the CPU PM notifier calls please, because
> > they are not needed).
> 
> OK no problem.  That's easy enough.  I added them to play it safe as a 
> test patch in case some VFP content could be lost somehow by looping 
> back through the CPU init code for example, and needed to be saved.

Ok, thanks.

> > I also said that's the only thing he could do, and I still think 
> > that's not a nice way to use the cpu_suspend API for something it was 
> > not designed for, that's what I wanted to say, period.
> 
> Well... Maybe it wasn't designed for that, but it certainly can be used 
> for that. And with no modifications to the core code, making this 
> solution fairly elegant.  This is not so different from, say, the BPF 
> code being reused for seccomp_filters. BPF wasn't designed for system 
> call filtering, but it happens to work well.

You defined yourself "not a small thing", you know what you are doing
and that's enough for me. Please respect that when I reviewed it I thought
that was a hack. cpu_suspend is being used for many things in the kernel
and consolidating that took a while, please comment the code, that's all I
am asking you.

> > If I am allowed to say something, here is a couple of thoughts.
> > 
> > 1) CCI snoops and DVM enablement are secure operations, to do them in
> >non-secure world this must be overriden in firmware. You can argue,
> >you can think whatever you want, that's a fact. So, to use this
> >code SMP bit in SCTLR and CCI enablement must become non-secure
> >operations. This is a boot requirement for MCPM, right or wrong
> >it is up to platform designers to judge. If CCI and SMP enablement
> >are secure operations, we should not start adding random SMC calls
> >in the kernel, since managing coherency in the kernel would become
> >problematic, with lots of platform quirks. We do not want that to
> >happen, and I think we all agree on this.
> 
> One could certainly question the need for so many controls handled in 
> secure world.  But that is not the point.
> 
> Here we're talking about MCPM.  That implies the kernel has control over 
> SCTLR.SMP and the CCI.  If those things aren't under the kernel's 
> control, then MCPM is of no use to you.

ACTLR.SMP for the sake of precision and it was my typo, sorry. That's
all I wanted to read, so nothing to add.

> Therefore, if you do want to use MCPM, then this implies the kernel has 
> access to the CCI. And if it has access to it, then it should turn it on 
> by itself in all cases to be consistent, not only in half of the cases.

I agree.

> > 2) (1) must be documented.
> 
> Absolutely.  But let's be coherent in the implementation so the 
> documentation is as simple as it can be.

Ditto.

> > 3) When I talked about consequences for CPUidle (implicit), I was referring
> >to all sort of hacks we had to come up to bring devices like SPC
> >(remember ? I remember very very well unfortunately for me), or whatever
> >power controller up in the kernel early, too early to fit in any
> >existing kernel device framework. There is still no solution to that, and
> >the only way that code can exist is in mach- code. Right or wrong,
> >that's a second fact and in my opinion that's not nice for the ARM
> >kernel.
> 
> I disagree.  This can perfectly be turned into driver code. If we need 
> it too early for existing kernel device framework to handle this 
> properly, then the solution is to extend the existing framework or 
> create another one specially for that purpose.  This may not be obvious 
> when TC2 is the first/only platform in that situation, but if more 
> platforms have the same need then it'll be easier to abstract 
> commonalities into a framework.
> 
> Saying that no framework exists today or/and upstream maintainers are 
> being difficult is _not_ a reason for throwing your hands up and e.g. 
> shoving all this code into firmware instead.

You have a point, as long as we are all aware and we do not forget this
is a major problem, not a minor one. I do want to see a consolidate
story for CPUidle for ARM and this bullet is definitely part of the
picture. On a side note, you made me smile, it sounded like I wanted
to bury SPC code in firmware or anywhere else as long as it is not in the
kernel, which in a way is a true statement since I abhor that code =)

> > 4) When I am talking about firmware I am talking about sequences that
> >are very close to HW (disabling C bit, cleaning caches, exiting
> >coherency). Erratas notwithstanding, they are being standardized at
> >ARM the best we can. They might even end up being implemented in HW
> >in the not so far future. I understand they are tricky, I understand
> >they take lots of time to implement t

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Mon, 9 Jun 2014, Lorenzo Pieralisi wrote:

> I commented on Nico's patch because I did not like how it was
> implemented (at least remove the CPU PM notifier calls please, because
> they are not needed).

OK no problem.  That's easy enough.  I added them to play it safe as a 
test patch in case some VFP content could be lost somehow by looping 
back through the CPU init code for example, and needed to be saved.

> I also said that's the only thing he could do, and I still think 
> that's not a nice way to use the cpu_suspend API for something it was 
> not designed for, that's what I wanted to say, period.

Well... Maybe it wasn't designed for that, but it certainly can be used 
for that. And with no modifications to the core code, making this 
solution fairly elegant.  This is not so different from, say, the BPF 
code being reused for seccomp_filters. BPF wasn't designed for system 
call filtering, but it happens to work well.

> If I am allowed to say something, here is a couple of thoughts.
> 
> 1) CCI snoops and DVM enablement are secure operations, to do them in
>non-secure world this must be overriden in firmware. You can argue,
>you can think whatever you want, that's a fact. So, to use this
>code SMP bit in SCTLR and CCI enablement must become non-secure
>operations. This is a boot requirement for MCPM, right or wrong
>it is up to platform designers to judge. If CCI and SMP enablement
>are secure operations, we should not start adding random SMC calls
>in the kernel, since managing coherency in the kernel would become
>problematic, with lots of platform quirks. We do not want that to
>happen, and I think we all agree on this.

One could certainly question the need for so many controls handled in 
secure world.  But that is not the point.

Here we're talking about MCPM.  That implies the kernel has control over 
SCTLR.SMP and the CCI.  If those things aren't under the kernel's 
control, then MCPM is of no use to you.

Therefore, if you do want to use MCPM, then this implies the kernel has 
access to the CCI. And if it has access to it, then it should turn it on 
by itself in all cases to be consistent, not only in half of the cases.

> 2) (1) must be documented.

Absolutely.  But let's be coherent in the implementation so the 
documentation is as simple as it can be.

> 3) When I talked about consequences for CPUidle (implicit), I was referring
>to all sort of hacks we had to come up to bring devices like SPC
>(remember ? I remember very very well unfortunately for me), or whatever
>power controller up in the kernel early, too early to fit in any
>existing kernel device framework. There is still no solution to that, and
>the only way that code can exist is in mach- code. Right or wrong,
>that's a second fact and in my opinion that's not nice for the ARM
>kernel.

I disagree.  This can perfectly be turned into driver code. If we need 
it too early for existing kernel device framework to handle this 
properly, then the solution is to extend the existing framework or 
create another one specially for that purpose.  This may not be obvious 
when TC2 is the first/only platform in that situation, but if more 
platforms have the same need then it'll be easier to abstract 
commonalities into a framework.

Saying that no framework exists today or/and upstream maintainers are 
being difficult is _not_ a reason for throwing your hands up and e.g. 
shoving all this code into firmware instead.

> 4) When I am talking about firmware I am talking about sequences that
>are very close to HW (disabling C bit, cleaning caches, exiting
>coherency). Erratas notwithstanding, they are being standardized at
>ARM the best we can. They might even end up being implemented in HW
>in the not so far future. I understand they are tricky, I understand
>they take lots of time to implement them and to debug them, what I
>want to say is that they are becoming standard and we _must_ reuse the
>same code for all ARM platforms. You can implement them in MCPM (see
>(1)) or in firmware (and please do not start painting me as firmware
>hugger here, I am referring to standard power down sequences that
>again, are very close to HW state machines 

That's where the disconnect lies.  On the one hand you say "I understand 
they are tricky, I understand they take lots of time to implement them 
and to debug them" and on the other hand you say "They might end up being 
implemented in HW in the not so far future."  That simply makes no 
economical sense at all!

When some operation is 1) tricky and takes time to debug, and 2) not 
performance critical (no one is trying to get in and out of idle or 
hibernation a billion times per second), then you should never ever put 
such a thing in firmware, and hardware should be completely out of the 
question!

>and more importantly if they
>HAVE to run in secure world that's the only solution 

Re: Problems booting exynos5420 with >1 CPU

[Lorenzo Pieralisi]

On Mon, Jun 09, 2014 at 09:47:42PM +0100, Kevin Hilman wrote:
> Nicolas Pitre  writes:
> 
> > On Sun, 8 Jun 2014, Lorenzo Pieralisi wrote:
> >
> >> On Sun, Jun 08, 2014 at 12:53:34AM +0100, Olof Johansson wrote:
> >> > Lorenzo,
> >> > 
> >> > Since you're emailing from @arm.com, some of this is to the wider
> >> > recipient and maybe not directly to you:
> >> 
> >> I am glad to reply and take blame since this is a debate definitely worth
> >> having.
> >
> > Great.  Because I would like to steer this debate a little towards the 
> > genuine cause rather than sticking to some particular consequences.

I commented on Nico's patch because I did not like how it was
implemented (at least remove the CPU PM notifier calls please, because
they are not needed). I also said that's the only thing he could do,
and I still think that's not a nice way to use the cpu_suspend API
for something it was not designed for, that's what I wanted to say,
period.

> >> Guys, do not get me wrong here. There are fixes that can be deemed
> >> acceptable in an OS, there are fixes that can't. I just can't help thinking
> >> that Nicolas' patch is a nasty hack (and I am far, really really far from
> >> blaming him for that, because that's the only patch that can fix that
> >> issue in the kernel), and he perfectly knows that.
> >
> > You know what?  The more I think about my patch, the more I consider 
> > this should be the standard way of setting up things unconditionally on 
> > _all_ platforms using MCPM.  Why? Because that's the most coherent thing 
> > to do!
> 
> I agree.
> 
> > I really think the kernel should either be responsible for the CCI or it 
> > should not at all.  And conversely for the bootloader.  Right now we 
> > have an implicit requirement that the bootloader should turn on the CCI, 
> > but only for cold boot, and only for the boot cluster, and not for CPU 
> > resuming from idle, and what other case we haven't thought about yet.  
> > And as noticed this requirement is not documented.
> 
> In addition to being a firmware minimalist like Nico, what I find most
> objectional to the bootloader approach is that even with CCI enabled by
> the firmware, since it's a runtime requirement (for low-power idle or
> suspend), the kernel has to handle it anyways.  So you end up with a
> partial solution in the firwmare (for boot cluster only) *and* a full
> solution in the kernel.  This doesn't make any sense, expecially because
> the kernel might then have to do things differently on cold boot
> vs. low-power idle/suspend or differently on the boot cluster vs. other
> clusters.  From a maintenance PoV, this is a mess and could easily lead
> to just as many SoC specific hacks that are different across platforms.
> 
> Stated more simply: If the kernel has to manage the resource at runtime
> due to low-power idle/suspend.  I don't see any reason why it shouldn't
> manage it at cold boot time also.

If I am allowed to say something, here is a couple of thoughts.

1) CCI snoops and DVM enablement are secure operations, to do them in
   non-secure world this must be overriden in firmware. You can argue,
   you can think whatever you want, that's a fact. So, to use this
   code SMP bit in SCTLR and CCI enablement must become non-secure
   operations. This is a boot requirement for MCPM, right or wrong
   it is up to platform designers to judge. If CCI and SMP enablement
   are secure operations, we should not start adding random SMC calls
   in the kernel, since managing coherency in the kernel would become
   problematic, with lots of platform quirks. We do not want that to
   happen, and I think we all agree on this.
2) (1) must be documented.
3) When I talked about consequences for CPUidle (implicit), I was referring
   to all sort of hacks we had to come up to bring devices like SPC
   (remember ? I remember very very well unfortunately for me), or whatever
   power controller up in the kernel early, too early to fit in any
   existing kernel device framework. There is still no solution to that, and
   the only way that code can exist is in mach- code. Right or wrong,
   that's a second fact and in my opinion that's not nice for the ARM
   kernel.
4) When I am talking about firmware I am talking about sequences that
   are very close to HW (disabling C bit, cleaning caches, exiting
   coherency). Erratas notwithstanding, they are being standardized at
   ARM the best we can. They might even end up being implemented in HW
   in the not so far future. I understand they are tricky, I understand
   they take lots of time to implement them and to debug them, what I
   want to say is that they are becoming standard and we _must_ reuse the
   same code for all ARM platforms. You can implement them in MCPM (see
   (1)) or in firmware (and please do not start painting me as firmware
   hugger here, I am referring to standard power down sequences that
   again, are very close to HW state machines and more importantly if they
   HAVE to

Re: Problems booting exynos5420 with >1 CPU

[Kevin Hilman]

Nicolas Pitre  writes:

> On Sun, 8 Jun 2014, Lorenzo Pieralisi wrote:
>
>> On Sun, Jun 08, 2014 at 12:53:34AM +0100, Olof Johansson wrote:
>> > Lorenzo,
>> > 
>> > Since you're emailing from @arm.com, some of this is to the wider
>> > recipient and maybe not directly to you:
>> 
>> I am glad to reply and take blame since this is a debate definitely worth
>> having.
>
> Great.  Because I would like to steer this debate a little towards the 
> genuine cause rather than sticking to some particular consequences.
>
>> Guys, do not get me wrong here. There are fixes that can be deemed
>> acceptable in an OS, there are fixes that can't. I just can't help thinking
>> that Nicolas' patch is a nasty hack (and I am far, really really far from
>> blaming him for that, because that's the only patch that can fix that
>> issue in the kernel), and he perfectly knows that.
>
> You know what?  The more I think about my patch, the more I consider 
> this should be the standard way of setting up things unconditionally on 
> _all_ platforms using MCPM.  Why? Because that's the most coherent thing 
> to do!

I agree.

> I really think the kernel should either be responsible for the CCI or it 
> should not at all.  And conversely for the bootloader.  Right now we 
> have an implicit requirement that the bootloader should turn on the CCI, 
> but only for cold boot, and only for the boot cluster, and not for CPU 
> resuming from idle, and what other case we haven't thought about yet.  
> And as noticed this requirement is not documented.

In addition to being a firmware minimalist like Nico, what I find most
objectional to the bootloader approach is that even with CCI enabled by
the firmware, since it's a runtime requirement (for low-power idle or
suspend), the kernel has to handle it anyways.  So you end up with a
partial solution in the firwmare (for boot cluster only) *and* a full
solution in the kernel.  This doesn't make any sense, expecially because
the kernel might then have to do things differently on cold boot
vs. low-power idle/suspend or differently on the boot cluster vs. other
clusters.  From a maintenance PoV, this is a mess and could easily lead
to just as many SoC specific hacks that are different across platforms.

Stated more simply: If the kernel has to manage the resource at runtime
due to low-power idle/suspend.  I don't see any reason why it shouldn't
manage it at cold boot time also.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Doug Anderson]

Kevin and Nicolas,

On Mon, Jun 9, 2014 at 1:27 PM, Kevin Hilman  wrote:
> Nicolas Pitre  writes:
>
>> On Sat, 7 Jun 2014, Abhilash Kesavan wrote:
>>
>>> Hi Nicolas,
>>>
>>> The first man of the incoming cluster enables its snoops via the
>>> power_up_setup function. During secondary boot-up, this does not occur
>>> for the boot cluster. Hence, I enable the snoops for the boot cluster
>>> as a one-time setup from the u-boot prompt. After secondary boot-up
>>> there is no modification that I do.
>>
>> OK that's good.
>>
>>> Where should this be ideally done ?
>>
>> If I remember correctly, the CCI can be safely activated only when the
>> cache is disabled.  So that means the CCI should ideally be turned on
>> for the boot cluster (and *only* for the boot CPU) by the bootloader.
>>
>> Now... If you _really_ prefer to do it from the kernel to avoid
>> difficulties with bootloader updates, then it should be possible to do
>> it from the kernel by temporarily turning the cache off.  This is not a
>> small thing but the MCPM infrastructure can be leveraged.  Here's what I
>> tried on a TC2 which might just work for you as well:
>
> FWIW, I dropped the u-boot hack I was using to enable CCI and tested
> this patch (with a cut/paste of the TC2 specific stuff into
> mach-exynos/mcpm-exynos.c) along with Doug's patch[1] and
> and confirm that all 8 cores boot up on the Chromebook2 using linux-next.
>
> Kevin
>
> [1] 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/262440.html

Agreed.  Nicolas's patch 
plus the copy/paste to exynos made things boot for me, too.

-Doug

---

Reference of the copy/paste to exynos (though gmail is munging my tabs):

diff --git a/arch/arm/mach-exynos/mcpm-exynos.c
b/arch/arm/mach-exynos/mcpm-exynos.c
index ace0ed6..218b9ff 100644
--- a/arch/arm/mach-exynos/mcpm-exynos.c
+++ b/arch/arm/mach-exynos/mcpm-exynos.c
@@ -295,6 +295,25 @@ static const struct of_device_id exynos_dt_mcpm_match[] = {
{},
 };

+int mcpm_loopback(void (*cache_disable)(void));
+static void exynos_cache_down(void)
+{
+   pr_warn("exynos: disabling cache\n");
+   if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A15) {
+   /*
+* On the Cortex-A15 we need to disable
+* L2 prefetching before flushing the cache.
+*/
+   asm volatile(
+   "mcrp15, 1, %0, c15, c0, 3 \n\t"
+   "isb\n\t"
+   "dsb"
+   : : "r" (0x400) );
+   }
+   v7_exit_coherency_flush(all);
+   cci_disable_port_by_cpu(read_cpuid_mpidr());
+}
+
 static int __init exynos_mcpm_init(void)
 {
struct device_node *node;
@@ -336,6 +355,7 @@ static int __init exynos_mcpm_init(void)
iounmap(ns_sram_base_addr);
return ret;
}
+   BUG_ON(mcpm_loopback(exynos_cache_down) != 0);

mcpm_smp_set_ops();
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Kevin Hilman]

Nicolas Pitre  writes:

> On Sat, 7 Jun 2014, Abhilash Kesavan wrote:
>
>> Hi Nicolas,
>> 
>> The first man of the incoming cluster enables its snoops via the
>> power_up_setup function. During secondary boot-up, this does not occur
>> for the boot cluster. Hence, I enable the snoops for the boot cluster
>> as a one-time setup from the u-boot prompt. After secondary boot-up
>> there is no modification that I do.
>
> OK that's good.
>
>> Where should this be ideally done ?
>
> If I remember correctly, the CCI can be safely activated only when the 
> cache is disabled.  So that means the CCI should ideally be turned on 
> for the boot cluster (and *only* for the boot CPU) by the bootloader.
>
> Now... If you _really_ prefer to do it from the kernel to avoid 
> difficulties with bootloader updates, then it should be possible to do 
> it from the kernel by temporarily turning the cache off.  This is not a 
> small thing but the MCPM infrastructure can be leveraged.  Here's what I 
> tried on a TC2 which might just work for you as well:

FWIW, I dropped the u-boot hack I was using to enable CCI and tested
this patch (with a cut/paste of the TC2 specific stuff into
mach-exynos/mcpm-exynos.c) along with Doug's patch[1] and 
and confirm that all 8 cores boot up on the Chromebook2 using linux-next.

Kevin

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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Russell King - ARM Linux]

On Sun, Jun 08, 2014 at 02:55:03PM -0400, Nicolas Pitre wrote:
> On Sun, 8 Jun 2014, Russell King - ARM Linux wrote:
> > No, I was specifically thinking about the various iPAQ specific things
> > like the additional platform specific ATAGs that they invented with
> > zero reference to mainline, and then expected them to be accepted as-is.
> > 
> > I gave them a very clear message over that (which was "no way") and that
> > was essentially the last of their mainline submissions.
> 
> That's basically what I'm saying.  They were too lazy to fix their code.  
> But nothing prevented that otherwise. They certainly couldn't use the 
> "firmware is broken and too hard to update" excuse.

No, they just continued to use those ATAGs that they invented.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Sun, 8 Jun 2014, Russell King - ARM Linux wrote:

> On Sun, Jun 08, 2014 at 02:26:43PM -0400, Nicolas Pitre wrote:
> > On Sun, 8 Jun 2014, Russell King - ARM Linux wrote:
> > 
> > > On Sat, Jun 07, 2014 at 04:53:34PM -0700, Olof Johansson wrote:
> > > > You do realize that you have absolutely zero leverage over us on this,
> > > > right? Our product is already shipped with kernel code that fixes
> > > > this.
> > > 
> > > That is never a justification for forcing /any/ code into the kernel.
> > > 
> > > We've been here before with the iPAQs, where there were all sorts of
> > > horrid hacks that were in the code for that device, and we said no to
> > > it, and we kept it out of the mainline kernel, and stopped those hacks
> > > polluting elsewhere (because people got to know on the whole that if
> > > they used those hacks, it would bar them from mainline participation.)
> > 
> > That's different.  The iPaq had very little in terms of firmware, and 
> > the one it had was field upgradable with almost no risk to brick it 
> > (unless you wanted to hack the firmware code but that's another story).  
> > The reason iPaq had never been well supported in mainline can be 
> > attributed to laziness for not wanting to make the code into a shape 
> > acceptable for mainline inclusion.  But nothing fundamentally prevented 
> > that from happening if someone had wanted to do it.
> 
> No, I was specifically thinking about the various iPAQ specific things
> like the additional platform specific ATAGs that they invented with
> zero reference to mainline, and then expected them to be accepted as-is.
> 
> I gave them a very clear message over that (which was "no way") and that
> was essentially the last of their mainline submissions.

That's basically what I'm saying.  They were too lazy to fix their code.  
But nothing prevented that otherwise. They certainly couldn't use the 
"firmware is broken and too hard to update" excuse.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Russell King - ARM Linux]

On Sun, Jun 08, 2014 at 02:26:43PM -0400, Nicolas Pitre wrote:
> On Sun, 8 Jun 2014, Russell King - ARM Linux wrote:
> 
> > On Sat, Jun 07, 2014 at 04:53:34PM -0700, Olof Johansson wrote:
> > > You do realize that you have absolutely zero leverage over us on this,
> > > right? Our product is already shipped with kernel code that fixes
> > > this.
> > 
> > That is never a justification for forcing /any/ code into the kernel.
> > 
> > We've been here before with the iPAQs, where there were all sorts of
> > horrid hacks that were in the code for that device, and we said no to
> > it, and we kept it out of the mainline kernel, and stopped those hacks
> > polluting elsewhere (because people got to know on the whole that if
> > they used those hacks, it would bar them from mainline participation.)
> 
> That's different.  The iPaq had very little in terms of firmware, and 
> the one it had was field upgradable with almost no risk to brick it 
> (unless you wanted to hack the firmware code but that's another story).  
> The reason iPaq had never been well supported in mainline can be 
> attributed to laziness for not wanting to make the code into a shape 
> acceptable for mainline inclusion.  But nothing fundamentally prevented 
> that from happening if someone had wanted to do it.

No, I was specifically thinking about the various iPAQ specific things
like the additional platform specific ATAGs that they invented with
zero reference to mainline, and then expected them to be accepted as-is.

I gave them a very clear message over that (which was "no way") and that
was essentially the last of their mainline submissions.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Sun, 8 Jun 2014, Russell King - ARM Linux wrote:

> On Sat, Jun 07, 2014 at 04:53:34PM -0700, Olof Johansson wrote:
> > You do realize that you have absolutely zero leverage over us on this,
> > right? Our product is already shipped with kernel code that fixes
> > this.
> 
> That is never a justification for forcing /any/ code into the kernel.
> 
> We've been here before with the iPAQs, where there were all sorts of
> horrid hacks that were in the code for that device, and we said no to
> it, and we kept it out of the mainline kernel, and stopped those hacks
> polluting elsewhere (because people got to know on the whole that if
> they used those hacks, it would bar them from mainline participation.)

That's different.  The iPaq had very little in terms of firmware, and 
the one it had was field upgradable with almost no risk to brick it 
(unless you wanted to hack the firmware code but that's another story).  
The reason iPaq had never been well supported in mainline can be 
attributed to laziness for not wanting to make the code into a shape 
acceptable for mainline inclusion.  But nothing fundamentally prevented 
that from happening if someone had wanted to do it.

Here we're talking about firmware-induced hacks for which, had there 
been no firmware, the kernel would be in full position to fix properly 
because that would have been a kernel bug after all.

Hence my crusade against this "you should abstract things in firmware" 
mantra. Especially for mobile devices.

But, because firmware already exists out there and it is between 
prohibitive and impossible to fix, we have no choice but to compensate 
in the kernel some times.  In this very case my approach is to render 
the firmware irrelevant globally rather than trying to work around it 
for one particular device.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Sun, 8 Jun 2014, Lorenzo Pieralisi wrote:

> On Sun, Jun 08, 2014 at 12:53:34AM +0100, Olof Johansson wrote:
> > Lorenzo,
> > 
> > Since you're emailing from @arm.com, some of this is to the wider
> > recipient and maybe not directly to you:
> 
> I am glad to reply and take blame since this is a debate definitely worth
> having.

Great.  Because I would like to steer this debate a little towards the 
genuine cause rather than sticking to some particular consequences.

> Guys, do not get me wrong here. There are fixes that can be deemed
> acceptable in an OS, there are fixes that can't. I just can't help thinking
> that Nicolas' patch is a nasty hack (and I am far, really really far from
> blaming him for that, because that's the only patch that can fix that
> issue in the kernel), and he perfectly knows that.

You know what?  The more I think about my patch, the more I consider 
this should be the standard way of setting up things unconditionally on 
_all_ platforms using MCPM.  Why? Because that's the most coherent thing 
to do!

I really think the kernel should either be responsible for the CCI or it 
should not at all.  And conversely for the bootloader.  Right now we 
have an implicit requirement that the bootloader should turn on the CCI, 
but only for cold boot, and only for the boot cluster, and not for CPU 
resuming from idle, and what other case we haven't thought about yet.  
And as noticed this requirement is not documented.

> > > Whatever the outcome of this thread, a booting protocol update for CCI
> > > is in order, even if we have to debate it for 6 months or more to get
> > > an agreement.
> > 

And in the end I don't think the CCI should have to be documented as a 
boot requirement.  Instead of having firmware implementers understand 
when they should care about the CCI and when they shouldn't, I'd much 
prefer if they hadn't to care at all. I really prefer when 
responsibility for something is well encapsulated in one place and not 
shared across layers like the firmware or the kernel depending on some 
context. The complexity of a system (and therefore the probability for 
bugs) grows with the square of the number of interrelations between 
constituent parts. So we should always strive to make the boot protocol 
_simpler_ not more complex.

And if complete responsibility for the CCI in the kernel had been 
assumed from the beginning, we wouldn't be struggling in this power play 
to determine which side should give in.  Especially since the kernel has 
all the necessary infrastructure to do it all already, and I must say in 
a rather elegant manner.

> > I'm a very strong proponent of enabling upstream support for our
> > platform (for several reasons -- most of these are actually business
> > reasons for us, but also because it's the right thing to do). Finding
> > the trade-off for what workarounds are still reasonable to do in the
> > kernel for that situation is obviously hard and we're disagreeing. But
> > the scope for these workarounds is not large.

Will people ever realize that, if the kernel was more in control of the 
hardware (isn't that the role of an OS kernel to serve as the hardware 
abstraction layer?) then we wouldn't be talking about "workarounds" but 
rather "standard fixes"?

> > In this case, the change we're looking at is enabling the CCI port for
> > the boot cpu. It's perfectly containable in exynos-only code, and we
> > can surround it by however many comments of never ever using it as an
> > example for how to do it as you'll want.

In this case, to state my opinion clearly, it is the general design that 
was flawed and the kernel should be fixed to enable the CCI for the boot 
CPU itself _when_ it knows it is going to need it.  To start with, the 
bootloader has no need what so ever for using more than one CPU, unless 
it wants to become an operating system, so it shouldn't have to care at 
all.  The kernel, if booted without the CCI information in the DTB, will 
run with only one CPU and won't rely on the CCI.  Logically the CCI 
could be left turned off in that case, possibly increasing bus 
performance and saving some power.

> I agree with what you are saying, but if for any reason someone will
> copy that code to paper over yet another firmware quirk and think that's
> the right thing to do, that would be grave IMHO.

Someone shouldn't have to copy that code because I'm getting more and 
more convinced it should be made generic and unconditional, and by doing 
so removing any possibility for firmware to get that part wrong again.  
According to my quick experiment on TC2, this took only 271 microseconds 
to perform so this is not like if that would make a significant 
difference in boot time.

> > > I do not think they do. The kernel should not become a place where 
> > > firmware
> > > bugs are fixed, if you refuse to fix the bug and this code does not get
> > > upstream I am pretty sure next time more attention will be paid.

Again, this is coming ab

Re: Problems booting exynos5420 with >1 CPU

[Russell King - ARM Linux]

On Sun, Jun 08, 2014 at 01:45:30PM +0100, Lorenzo Pieralisi wrote:
> Olof, it is not puritanism, it is all about upstreaming code. If we
> keep accepting these hacks and we end up with mach code full of them
> we have a problem, do you agree ?

To see the kind of problem that accepting hacked up code can cause, you
only have to look at Olof's build logs to see the warnings from the L2x0
cache code, which I've been totally unable to complete the cleanup of
/because/ we've historically accepted hacks into it.

I think that the legacy code is just going to have to stay (and I don't
want the warnings papered over, because I /want/ that crap to stick out
like a sore thumb), until someone can get sufficient motivation to work
out how to finally unuse the old functions.

Had I been on top of the L2 cache code earlier, and prevented these hacks
from going in (insisting that it was done properly) we would not be in
this position today where no one seems to know to fix this stuff.

This is the whole point - and nicely illustrates how easy it is for code
to become unmaintainable by accepting hacks to it.  A bit of push-back at
code acceptance time helps to save us from these problems in the future.

So, what I would want to see is not only what Lorenzo is saying (the
disclaimer in the comments) but also a technical description it, so if
the code needs to be modified in the future, we don't end up in this
kind of situation wondering what the code is doing/why the code exists.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Lorenzo Pieralisi]

On Sun, Jun 08, 2014 at 12:53:34AM +0100, Olof Johansson wrote:
> Lorenzo,
> 
> Since you're emailing from @arm.com, some of this is to the wider
> recipient and maybe not directly to you:

I am glad to reply and take blame since this is a debate definitely worth
having.

[...]

> > Right, CCI snoops and DVMs for a cluster must be enabled by the first man
> > running in that cluster upon cluster power up with caches off, where the
> > first man must be arbitrated if multiple CPUs are racing for enabling CCI.
> > This is not an issue on cold boot, it is on resuming from CPUidle.
> 
> That's already handled by the MCPM backend on exynos:
> 
> /*
>  * Enable cluster-level coherency, in preparation for turning on the MMU.
>  */
> static void __naked exynos_pm_power_up_setup(unsigned int affinity_level)
> {
> asm volatile ("\n"
> "cmpr0, #1\n"
> "bxne   lr\n"
> "b  cci_enable_port_for_self");
> }
> 
> >> > True, TC2 on warm-boot reenables CCI, and that's because it runs the
> >> > kernel in secure world, and again that's _wrong_.
> >>
> >> Let me respectfully disagree.
> >
> > You are welcome =)
> >
> >> > It must be done in firmware, and I am totally against any attempt at
> >> > papering over what looks like a messed up firmware implementation with
> >> > a bunch of hacks in the kernel, because that's what the patch below is
> >> > (soft restarting a CPU to enable CCI ? and adding generic code for that ?
> >> > what's next ?)
> >>
> >> Are you promoting for the removal of drivers/bus/arm-cci.c ?
> >
> > I really do not want to go there, but I might (apart from CCI PMUs).
> >
> >> You do realize that the fundamental raison d'?tre for MCPM is actually
> >> to manage the race free enabling of the cache and CCI ?
> >
> > Yes I do and I was willing to help implement it for TC2 to provide people
> > with an example on how to do it _properly_ (in secure world though, and that
> > was a mistake IMHO). If what we get is a workaround for every platform
> > going upstream, we are in a bind, seriously.
> 
> Real life is messy. Bugs happen. Having a stance that the kernel has
> to be a puritan implementation that can be done in a vacuum where bugs
> in hardware and firmware don't exist (and are fixable in the right
> place every single case) is not realistic. Linux is a useless piece of
> software to us if that is the case.
> 
> I've seen this from several other ARM developers in the past. It's
> almost like they were a couple of degrees removed from actually
> working on shipping real products and dealing with real users.

Guys, do not get me wrong here. There are fixes that can be deemed
acceptable in an OS, there are fixes that can't. I just can't help thinking
that Nicolas' patch is a nasty hack (and I am far, really really far from
blaming him for that, because that's the only patch that can fix that
issue in the kernel), and he perfectly knows that.

> > Whatever the outcome of this thread, a booting protocol update for CCI
> > is in order, even if we have to debate it for 6 months or more to get
> > an agreement.
> 
> Ding ding ding. Current documentation is in some very complex C
> frameworks (MCPM), and some device tree bindings that obviously don't
> cover this. Real documentation is obviously needed, but more than that
> (see below).
> 
> I'd actually argue that you don't have a leg to stand on in your
> complaints at all because:
> 
> 1) There's no documentation of the requirements
> 2) The only existing golden platform (TC2) manages CCI similar to how
> exynos does.

1) Blame taken, nothing to say. That's why I mentioned that CCI enablement
   must be part of a boot protocol document to prevent this from happening
   again.

2) Apart from the boot cluster, but that's related to (1).

> >> > I understand there is an issue and lots at stake here, but I do not want 
> >> > the
> >> > patch below to be merged in the kernel, I am sorry, it is a tad too much.
> >>
> >> Lorenzo: Don't get me wrong.  The Chromebooks, and possibly to some
> >> extent some people at Samsung, were simply too confident in their
> >> ability to create absolutely bug-free firmware code to the point of not
> >> making its update easy in the field.  This is completely outrageous in
> >> my point of view.
> 
> I would like to clarify that firmware is updateable today. But
> Chromebooks are in many ways vertically integrated products, so if a
> problem is found, there's always going to be a trade-off between the
> places to fix it for our own product.
> 
> I have successfully argued for fixes in firmware (or EC) for some bugs
> in the past. For others we have to go with the cheaper fix. And at the
> end of the day, pushing out a new firmware is a massive undertaking
> compared to a kernel workaround, and doing so only because upstream
> maintainers aren't happy with the way we and Samsung solved something
> in our firmware+kernel combo is an argument that is hard to win -- it
> doesn't affect the product

Re: Problems booting exynos5420 with >1 CPU

[Olof Johansson]

On Sat, Jun 7, 2014 at 5:19 PM, Russell King - ARM Linux
 wrote:
> On Sat, Jun 07, 2014 at 04:53:34PM -0700, Olof Johansson wrote:
>> You do realize that you have absolutely zero leverage over us on this,
>> right? Our product is already shipped with kernel code that fixes
>> this.
>
> That is never a justification for forcing /any/ code into the kernel.

I'm not looking to force code in, I'm just making it clear that we
have limited chance to motivate rework of this since the primary
objective of the platform has already been met: We've shipped a
product with it.

I also don't think it's really in anyone's best interest to have to
open up their device, remove write protect, download and build a
mainline u-boot and try flashing that onto the system to get it to a
state where they can use a mainline kernel. There's too much risk of
bricking your hardware if you get it wrong, and you void your
warranty.

> We've been here before with the iPAQs, where there were all sorts of
> horrid hacks that were in the code for that device, and we said no to
> it, and we kept it out of the mainline kernel, and stopped those hacks
> polluting elsewhere (because people got to know on the whole that if
> they used those hacks, it would bar them from mainline participation.)

Unfortunately, very few companies actually care about mainline
participation today to the point where I don't think they care much
any set examples. :(

> There's many other instances.  Think about it - if we allowed this as
> an acceptance criteria, then all that vendors have to do to get their
> code into the kernel is change their development cycle: develop
> product, ship product, force code into mainline as a done deal not
> accepting any review comments back.

That is of course not a suitable way of working either. Unfortunately
what is mostly happening today is that vendors are doing this: develop
product, ship product. The last step never happens. Finding a middle
ground where we can pick up some of the platform quirks without making
the kernel a big ball of hacks is of course the tricky part.

In this specific case, we're not ignoring review feedback, and the
comments we've gotten we will absolutely make sure are fixed in the
next generation of product (if/when we do another big.LITTLE platform,
etc). There's just no room to fix it for the current generation. In
hindsight, of course what should have happened is that we should have
pushed the vendor to upstream the code sooner, and we're working on
making that better in the future too.


Since we're talking about upstreaming of platform support, this is
something I'm quite passionate about so I'll rant a bit:

Looking at the general case more than just this specific instance of
Samsung Chromebooks, I think we should in general work hard (where
vendors are willing to cooperate) to make sure that we can fully
enable support for platforms to the point where they can just run
unmodified upstream. Too many of the products today aren't shipping
kernels anywhere near what's mainline: It's usually a kernel that is a
couple of years old with a few thousand patches on top. It means that
bugfixes for the platforms (and much other useful code) doesn't find
its way into the kernel, and we have a long (or nonexistent) cycle of
feedback due to it. Drivers are in some cases completely broken
because nobody actually uses the upstream versions, people post
building-but-broken code because they can't actually run and test the
patch on any existing hardware with mainline so they just forward-port
what they have in the product tree and hope for the best. Etc.

I would really like to reach a state where we have several substantial
and popular platforms that work well enough with mainline that people
can use some of the mainline-near distros to run on the machine.
Cubox-i is a great example, even though there are still some pieces
left there as well. The new Chromebooks have hardware specs that are
quite suitable to use as native development platforms (good CPUs, 4GB
ram, and micro-SD card to expand beyond the basic 16GB eMMC), so I
think it makes sense to try to enable them and pick up a minimal set
of the less than ideal code snippets for that. We'll end up with a
better supported and more solid kernel if we can get distros such as
Fedora and Debian to ship with these upstream kernels instead of the
vendor tree (which is 3.8 based in this case, and won't ever move
forward).


-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Russell King - ARM Linux]

On Sat, Jun 07, 2014 at 04:53:34PM -0700, Olof Johansson wrote:
> You do realize that you have absolutely zero leverage over us on this,
> right? Our product is already shipped with kernel code that fixes
> this.

That is never a justification for forcing /any/ code into the kernel.

We've been here before with the iPAQs, where there were all sorts of
horrid hacks that were in the code for that device, and we said no to
it, and we kept it out of the mainline kernel, and stopped those hacks
polluting elsewhere (because people got to know on the whole that if
they used those hacks, it would bar them from mainline participation.)

There's many other instances.  Think about it - if we allowed this as
an acceptance criteria, then all that vendors have to do to get their
code into the kernel is change their development cycle: develop
product, ship product, force code into mainline as a done deal not
accepting any review comments back.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Olof Johansson]

Lorenzo,

Since you're emailing from @arm.com, some of this is to the wider
recipient and maybe not directly to you:

On Sat, Jun 7, 2014 at 3:36 PM, Lorenzo Pieralisi
 wrote:
> On Sat, Jun 07, 2014 at 09:06:36PM +0100, Nicolas Pitre wrote:
>> On Sat, 7 Jun 2014, Lorenzo Pieralisi wrote:
>>
>> > On Sat, Jun 07, 2014 at 05:10:27PM +0100, Nicolas Pitre wrote:
>> > > On Sat, 7 Jun 2014, Abhilash Kesavan wrote:
>> > >
>> > > > Hi Nicolas,
>> > > >
>> > > > The first man of the incoming cluster enables its snoops via the
>> > > > power_up_setup function. During secondary boot-up, this does not occur
>> > > > for the boot cluster. Hence, I enable the snoops for the boot cluster
>> > > > as a one-time setup from the u-boot prompt. After secondary boot-up
>> > > > there is no modification that I do.
>> > >
>> > > OK that's good.
>> > >
>> > > > Where should this be ideally done ?
>> > >
>> > > If I remember correctly, the CCI can be safely activated only when the
>> > > cache is disabled.  So that means the CCI should ideally be turned on
>> > > for the boot cluster (and *only* for the boot CPU) by the bootloader.
>> >
>> > CCI ports are enabled per-cluster, so the boot loader must turn on
>> > CCI for all clusters before the respective CPUs have a chance to
>> > turn on their caches. It is a secure operation, this can be overriden
>> > and probably that's what has been done, wrongly.
>>
>> Careful.  By saying "for all clusters" you might be interpreted as
>> saying that the CCI must be turned on even for CPUs that are not powered
>> up.
>
> Right, CCI snoops and DVMs for a cluster must be enabled by the first man
> running in that cluster upon cluster power up with caches off, where the
> first man must be arbitrated if multiple CPUs are racing for enabling CCI.
> This is not an issue on cold boot, it is on resuming from CPUidle.

That's already handled by the MCPM backend on exynos:

/*
 * Enable cluster-level coherency, in preparation for turning on the MMU.
 */
static void __naked exynos_pm_power_up_setup(unsigned int affinity_level)
{
asm volatile ("\n"
"cmpr0, #1\n"
"bxne   lr\n"
"b  cci_enable_port_for_self");
}

>> > True, TC2 on warm-boot reenables CCI, and that's because it runs the
>> > kernel in secure world, and again that's _wrong_.
>>
>> Let me respectfully disagree.
>
> You are welcome =)
>
>> > It must be done in firmware, and I am totally against any attempt at
>> > papering over what looks like a messed up firmware implementation with
>> > a bunch of hacks in the kernel, because that's what the patch below is
>> > (soft restarting a CPU to enable CCI ? and adding generic code for that ?
>> > what's next ?)
>>
>> Are you promoting for the removal of drivers/bus/arm-cci.c ?
>
> I really do not want to go there, but I might (apart from CCI PMUs).
>
>> You do realize that the fundamental raison d'?tre for MCPM is actually
>> to manage the race free enabling of the cache and CCI ?
>
> Yes I do and I was willing to help implement it for TC2 to provide people
> with an example on how to do it _properly_ (in secure world though, and that
> was a mistake IMHO). If what we get is a workaround for every platform
> going upstream, we are in a bind, seriously.

Real life is messy. Bugs happen. Having a stance that the kernel has
to be a puritan implementation that can be done in a vacuum where bugs
in hardware and firmware don't exist (and are fixable in the right
place every single case) is not realistic. Linux is a useless piece of
software to us if that is the case.

I've seen this from several other ARM developers in the past. It's
almost like they were a couple of degrees removed from actually
working on shipping real products and dealing with real users.

> Whatever the outcome of this thread, a booting protocol update for CCI
> is in order, even if we have to debate it for 6 months or more to get
> an agreement.

Ding ding ding. Current documentation is in some very complex C
frameworks (MCPM), and some device tree bindings that obviously don't
cover this. Real documentation is obviously needed, but more than that
(see below).

I'd actually argue that you don't have a leg to stand on in your
complaints at all because:

1) There's no documentation of the requirements
2) The only existing golden platform (TC2) manages CCI similar to how
exynos does.

>> > I understand there is an issue and lots at stake here, but I do not want 
>> > the
>> > patch below to be merged in the kernel, I am sorry, it is a tad too much.
>>
>> Lorenzo: Don't get me wrong.  The Chromebooks, and possibly to some
>> extent some people at Samsung, were simply too confident in their
>> ability to create absolutely bug-free firmware code to the point of not
>> making its update easy in the field.  This is completely outrageous in
>> my point of view.

I would like to clarify that firmware is updateable today. But
Chromebooks are in many ways vertically integrated products, so if a
problem is f

Re: Problems booting exynos5420 with >1 CPU

[Lorenzo Pieralisi]

On Sat, Jun 07, 2014 at 09:06:36PM +0100, Nicolas Pitre wrote:
> On Sat, 7 Jun 2014, Lorenzo Pieralisi wrote:
> 
> > On Sat, Jun 07, 2014 at 05:10:27PM +0100, Nicolas Pitre wrote:
> > > On Sat, 7 Jun 2014, Abhilash Kesavan wrote:
> > > 
> > > > Hi Nicolas,
> > > > 
> > > > The first man of the incoming cluster enables its snoops via the
> > > > power_up_setup function. During secondary boot-up, this does not occur
> > > > for the boot cluster. Hence, I enable the snoops for the boot cluster
> > > > as a one-time setup from the u-boot prompt. After secondary boot-up
> > > > there is no modification that I do.
> > > 
> > > OK that's good.
> > > 
> > > > Where should this be ideally done ?
> > > 
> > > If I remember correctly, the CCI can be safely activated only when the 
> > > cache is disabled.  So that means the CCI should ideally be turned on 
> > > for the boot cluster (and *only* for the boot CPU) by the bootloader.
> > 
> > CCI ports are enabled per-cluster, so the boot loader must turn on
> > CCI for all clusters before the respective CPUs have a chance to
> > turn on their caches. It is a secure operation, this can be overriden
> > and probably that's what has been done, wrongly.
> 
> Careful.  By saying "for all clusters" you might be interpreted as 
> saying that the CCI must be turned on even for CPUs that are not powered 
> up.

Right, CCI snoops and DVMs for a cluster must be enabled by the first man
running in that cluster upon cluster power up with caches off, where the
first man must be arbitrated if multiple CPUs are racing for enabling CCI.
This is not an issue on cold boot, it is on resuming from CPUidle.

> > True, TC2 on warm-boot reenables CCI, and that's because it runs the
> > kernel in secure world, and again that's _wrong_.
> 
> Let me respectfully disagree.

You are welcome =)

> > It must be done in firmware, and I am totally against any attempt at
> > papering over what looks like a messed up firmware implementation with
> > a bunch of hacks in the kernel, because that's what the patch below is
> > (soft restarting a CPU to enable CCI ? and adding generic code for that ?
> > what's next ?)
> 
> Are you promoting for the removal of drivers/bus/arm-cci.c ?

I really do not want to go there, but I might (apart from CCI PMUs).

> You do realize that the fundamental raison d'?tre for MCPM is actually 
> to manage the race free enabling of the cache and CCI ?

Yes I do and I was willing to help implement it for TC2 to provide people
with an example on how to do it _properly_ (in secure world though, and that
was a mistake IMHO). If what we get is a workaround for every platform
going upstream, we are in a bind, seriously.

Whatever the outcome of this thread, a booting protocol update for CCI
is in order, even if we have to debate it for 6 months or more to get
an agreement.

> > I understand there is an issue and lots at stake here, but I do not want the
> > patch below to be merged in the kernel, I am sorry, it is a tad too much.
> 
> Lorenzo: Don't get me wrong.  The Chromebooks, and possibly to some 
> extent some people at Samsung, were simply too confident in their 
> ability to create absolutely bug-free firmware code to the point of not 
> making its update easy in the field.  This is completely outrageous in 
> my point of view.  Yet one of the reactions was to call upstream kernel 
> people as purists because the kernel is so much easier to modify in 
> order to cover their mess and yet that might not be accepted.  Like I 
> said I won't stop shaming them publicly for their own "incompetence" 
> just yet (no pun intended), but being excessively purist does not 
> benefit anyone either, and for that they have a point.

I do not think they do. The kernel should not become a place where firmware
bugs are fixed, if you refuse to fix the bug and this code does not get
upstream I am pretty sure next time more attention will be paid.

Booting, powering up and down cores are standard procedures, why can't we
share the same code for all v7 platforms ? Why ?

And we are not talking about a race condition hit every 10 gazillions
cycles here.

> *HOWEVER* I have no choice but to say that many people at ARM, including 
> a couple individuals for whom I nevertheless have a lot of admiration, 
> also have an incredible faith in their ability to convince themselves, 
> and then turn around to preach to the world, that *more firmware* is 
> going to be so much purer and solve so many more problems than it 
> creates and become such a magical success that we should immediately 
> dedicate our soul to the cause with no hint of a doubt.
> 
> I'm sorry to rain on your parade, but I don't believe in this one iota.
> 
> Let me repeat the MCPM story again: it took 3 people, including 2 from 
> ARM, over *six* months to get everything right and stable on TC2. I 
> think you also contributed to that effort as well. Subsequent MCPM 
> backend contributions (yes, just the backend and not the core code)

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Sat, 7 Jun 2014, Lorenzo Pieralisi wrote:

> On Sat, Jun 07, 2014 at 05:10:27PM +0100, Nicolas Pitre wrote:
> > On Sat, 7 Jun 2014, Abhilash Kesavan wrote:
> > 
> > > Hi Nicolas,
> > > 
> > > The first man of the incoming cluster enables its snoops via the
> > > power_up_setup function. During secondary boot-up, this does not occur
> > > for the boot cluster. Hence, I enable the snoops for the boot cluster
> > > as a one-time setup from the u-boot prompt. After secondary boot-up
> > > there is no modification that I do.
> > 
> > OK that's good.
> > 
> > > Where should this be ideally done ?
> > 
> > If I remember correctly, the CCI can be safely activated only when the 
> > cache is disabled.  So that means the CCI should ideally be turned on 
> > for the boot cluster (and *only* for the boot CPU) by the bootloader.
> 
> CCI ports are enabled per-cluster, so the boot loader must turn on
> CCI for all clusters before the respective CPUs have a chance to
> turn on their caches. It is a secure operation, this can be overriden
> and probably that's what has been done, wrongly.

Careful.  By saying "for all clusters" you might be interpreted as 
saying that the CCI must be turned on even for CPUs that are not powered 
up.

> True, TC2 on warm-boot reenables CCI, and that's because it runs the
> kernel in secure world, and again that's _wrong_.

Let me respectfully disagree.

> It must be done in firmware, and I am totally against any attempt at
> papering over what looks like a messed up firmware implementation with
> a bunch of hacks in the kernel, because that's what the patch below is
> (soft restarting a CPU to enable CCI ? and adding generic code for that ?
> what's next ?)

Are you promoting for the removal of drivers/bus/arm-cci.c ?

You do realize that the fundamental raison d'être for MCPM is actually 
to manage the race free enabling of the cache and CCI ?

> I understand there is an issue and lots at stake here, but I do not want the
> patch below to be merged in the kernel, I am sorry, it is a tad too much.

Lorenzo: Don't get me wrong.  The Chromebooks, and possibly to some 
extent some people at Samsung, were simply too confident in their 
ability to create absolutely bug-free firmware code to the point of not 
making its update easy in the field.  This is completely outrageous in 
my point of view.  Yet one of the reactions was to call upstream kernel 
people as purists because the kernel is so much easier to modify in 
order to cover their mess and yet that might not be accepted.  Like I 
said I won't stop shaming them publicly for their own "incompetence" 
just yet (no pun intended), but being excessively purist does not 
benefit anyone either, and for that they have a point.

*HOWEVER* I have no choice but to say that many people at ARM, including 
a couple individuals for whom I nevertheless have a lot of admiration, 
also have an incredible faith in their ability to convince themselves, 
and then turn around to preach to the world, that *more firmware* is 
going to be so much purer and solve so many more problems than it 
creates and become such a magical success that we should immediately 
dedicate our soul to the cause with no hint of a doubt.

I'm sorry to rain on your parade, but I don't believe in this one iota.

Let me repeat the MCPM story again: it took 3 people, including 2 from 
ARM, over *six* months to get everything right and stable on TC2. I 
think you also contributed to that effort as well. Subsequent MCPM 
backend contributions (yes, just the backend and not the core code) took 
at least *five* rounds of reviews in one case, and after *seven* rounds 
in another case it is still not right, despite the publicly available 
TC2 implementation to serve as a reference.

I'm sure each time a new patch set was posted, their authors honestly 
believed their code was correct.  Otherwise why would they post buggy 
code?

Now you are telling me that they should have put that code into firmware 
instead?  Can you realize what a catastrophe this would have been? Are 
you _seriously_ believing that they would be up to their 5th firmware 
revision by now?  And that updating their firmware six months after 
product launch would be as easy as updating the kernel?

Software ALWAYS has bugs, whether it is user apps, the kernel, firmware 
or boot ROM. The bigger one of those parts is, the more bugs it will 
have. And the cost to vendors for fixing those bugs grow exponentially 
down each level. For proof, we're now considering possible workarounds 
in the kernel to sidestep the difficulty with updating the firmware on a 
Chromebook.

Yet you're saying that firmware should grow code with the same 
complexity as the MCPM core, plus a machine specific backend that 
experience has proven multiple times is evidently hard to get right, 
into firmware because running Linux in secure mode is wrong?  If so we 
don't live in the same world indeed.

The day I see a firmware architecture that allows for 

Re: Problems booting exynos5420 with >1 CPU

[Lorenzo Pieralisi]

On Sat, Jun 07, 2014 at 05:10:27PM +0100, Nicolas Pitre wrote:
> On Sat, 7 Jun 2014, Abhilash Kesavan wrote:
> 
> > Hi Nicolas,
> > 
> > The first man of the incoming cluster enables its snoops via the
> > power_up_setup function. During secondary boot-up, this does not occur
> > for the boot cluster. Hence, I enable the snoops for the boot cluster
> > as a one-time setup from the u-boot prompt. After secondary boot-up
> > there is no modification that I do.
> 
> OK that's good.
> 
> > Where should this be ideally done ?
> 
> If I remember correctly, the CCI can be safely activated only when the 
> cache is disabled.  So that means the CCI should ideally be turned on 
> for the boot cluster (and *only* for the boot CPU) by the bootloader.

CCI ports are enabled per-cluster, so the boot loader must turn on
CCI for all clusters before the respective CPUs have a chance to
turn on their caches. It is a secure operation, this can be overriden
and probably that's what has been done, wrongly.

True, TC2 on warm-boot reenables CCI, and that's because it runs the
kernel in secure world, and again that's _wrong_.

It must be done in firmware, and I am totally against any attempt at
papering over what looks like a messed up firmware implementation with
a bunch of hacks in the kernel, because that's what the patch below is
(soft restarting a CPU to enable CCI ? and adding generic code for that ?
what's next ?)

I understand there is an issue and lots at stake here, but I do not want the
patch below to be merged in the kernel, I am sorry, it is a tad too much.

Lorenzo

> 
> Now... If you _really_ prefer to do it from the kernel to avoid 
> difficulties with bootloader updates, then it should be possible to do 
> it from the kernel by temporarily turning the cache off.  This is not a 
> small thing but the MCPM infrastructure can be leveraged.  Here's what I 
> tried on a TC2 which might just work for you as well:
> 
> diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
> index 86fd60fefb..1cc49de308 100644
> --- a/arch/arm/common/mcpm_entry.c
> +++ b/arch/arm/common/mcpm_entry.c
> @@ -12,11 +12,13 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  extern unsigned long 
> mcpm_entry_vectors[MAX_NR_CLUSTERS][MAX_CPUS_PER_CLUSTER];
>  
> @@ -146,6 +148,44 @@ int mcpm_cpu_powered_up(void)
>   return 0;
>  }
>  
> +static int go_down(unsigned long _arg)
> +{
> + void (*cache_disable)(void) = (void *)_arg;
> + unsigned int mpidr = read_cpuid_mpidr();
> + unsigned int cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> + unsigned int cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> + phys_reset_t phys_reset;
> +
> + mcpm_set_entry_vector(cpu, cluster, cpu_resume);
> + setup_mm_for_reboot();
> +
> + __mcpm_cpu_going_down(cpu, cluster);
> + BUG_ON(!__mcpm_outbound_enter_critical(cpu, cluster));
> + cache_disable();
> + __mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
> + __mcpm_cpu_down(cpu, cluster);
> +
> + phys_reset = (phys_reset_t)(unsigned long)virt_to_phys(cpu_reset);
> + phys_reset(virt_to_phys(mcpm_entry_point));
> + BUG();
> +}
> + 
> +int mcpm_loopback(void (*cache_disable)(void))
> +{
> + int ret;
> +
> + local_irq_disable();
> + local_fiq_disable();
> + ret = cpu_pm_enter();
> + if (!ret) {
> + ret = cpu_suspend((unsigned long)cache_disable, go_down);
> + cpu_pm_exit();
> + }
> + local_fiq_enable();
> + local_irq_enable();
> + return ret;
> +}
> +
>  struct sync_struct mcpm_sync;
>  
>  /*
> diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
> index 29e7785a54..abecdd734f 100644
> --- a/arch/arm/mach-vexpress/tc2_pm.c
> +++ b/arch/arm/mach-vexpress/tc2_pm.c
> @@ -323,6 +323,26 @@ static void __naked tc2_pm_power_up_setup(unsigned int 
> affinity_level)
>  "b   cci_enable_port_for_self ");
>  }
>  
> +int mcpm_loopback(void (*cache_disable)(void));
> +
> +static void tc2_cache_down(void)
> +{
> + pr_warn("TC2: disabling cache\n");
> + if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A15) {
> + /*
> +  * On the Cortex-A15 we need to disable
> +  * L2 prefetching before flushing the cache.
> +  */
> + asm volatile(
> + "mcrp15, 1, %0, c15, c0, 3 \n\t"
> + "isb\n\t"
> + "dsb"
> + : : "r" (0x400) );
> + }
> + v7_exit_coherency_flush(all);
> + cci_disable_port_by_cpu(read_cpuid_mpidr());
> +}
> +
>  static int __init tc2_pm_init(void)
>  {
>   int ret, irq;
> @@ -370,6 +390,7 @@ static int __init tc2_pm_init(void)
>   ret = mcpm_platform_register(&tc2_pm_power_ops);
>   if (!ret) {
>   mcpm_sync_init(tc2_pm_power_up_setup);
> + BUG_ON(mcpm_loopback(tc2_cache_down) != 0);
>   pr_info(

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Sat, 7 Jun 2014, Abhilash Kesavan wrote:

> Hi Nicolas,
> 
> The first man of the incoming cluster enables its snoops via the
> power_up_setup function. During secondary boot-up, this does not occur
> for the boot cluster. Hence, I enable the snoops for the boot cluster
> as a one-time setup from the u-boot prompt. After secondary boot-up
> there is no modification that I do.

OK that's good.

> Where should this be ideally done ?

If I remember correctly, the CCI can be safely activated only when the 
cache is disabled.  So that means the CCI should ideally be turned on 
for the boot cluster (and *only* for the boot CPU) by the bootloader.

Now... If you _really_ prefer to do it from the kernel to avoid 
difficulties with bootloader updates, then it should be possible to do 
it from the kernel by temporarily turning the cache off.  This is not a 
small thing but the MCPM infrastructure can be leveraged.  Here's what I 
tried on a TC2 which might just work for you as well:

diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
index 86fd60fefb..1cc49de308 100644
--- a/arch/arm/common/mcpm_entry.c
+++ b/arch/arm/common/mcpm_entry.c
@@ -12,11 +12,13 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
 #include 
 #include 
+#include 
 
 extern unsigned long mcpm_entry_vectors[MAX_NR_CLUSTERS][MAX_CPUS_PER_CLUSTER];
 
@@ -146,6 +148,44 @@ int mcpm_cpu_powered_up(void)
return 0;
 }
 
+static int go_down(unsigned long _arg)
+{
+   void (*cache_disable)(void) = (void *)_arg;
+   unsigned int mpidr = read_cpuid_mpidr();
+   unsigned int cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+   unsigned int cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+   phys_reset_t phys_reset;
+
+   mcpm_set_entry_vector(cpu, cluster, cpu_resume);
+   setup_mm_for_reboot();
+
+   __mcpm_cpu_going_down(cpu, cluster);
+   BUG_ON(!__mcpm_outbound_enter_critical(cpu, cluster));
+   cache_disable();
+   __mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
+   __mcpm_cpu_down(cpu, cluster);
+
+   phys_reset = (phys_reset_t)(unsigned long)virt_to_phys(cpu_reset);
+   phys_reset(virt_to_phys(mcpm_entry_point));
+   BUG();
+}
+   
+int mcpm_loopback(void (*cache_disable)(void))
+{
+   int ret;
+
+   local_irq_disable();
+   local_fiq_disable();
+   ret = cpu_pm_enter();
+   if (!ret) {
+   ret = cpu_suspend((unsigned long)cache_disable, go_down);
+   cpu_pm_exit();
+   }
+   local_fiq_enable();
+   local_irq_enable();
+   return ret;
+}
+
 struct sync_struct mcpm_sync;
 
 /*
diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
index 29e7785a54..abecdd734f 100644
--- a/arch/arm/mach-vexpress/tc2_pm.c
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -323,6 +323,26 @@ static void __naked tc2_pm_power_up_setup(unsigned int 
affinity_level)
 "  b   cci_enable_port_for_self ");
 }
 
+int mcpm_loopback(void (*cache_disable)(void));
+
+static void tc2_cache_down(void)
+{
+   pr_warn("TC2: disabling cache\n");
+   if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A15) {
+   /*
+* On the Cortex-A15 we need to disable
+* L2 prefetching before flushing the cache.
+*/
+   asm volatile(
+   "mcrp15, 1, %0, c15, c0, 3 \n\t"
+   "isb\n\t"
+   "dsb"
+   : : "r" (0x400) );
+   }
+   v7_exit_coherency_flush(all);
+   cci_disable_port_by_cpu(read_cpuid_mpidr());
+}
+
 static int __init tc2_pm_init(void)
 {
int ret, irq;
@@ -370,6 +390,7 @@ static int __init tc2_pm_init(void)
ret = mcpm_platform_register(&tc2_pm_power_ops);
if (!ret) {
mcpm_sync_init(tc2_pm_power_up_setup);
+   BUG_ON(mcpm_loopback(tc2_cache_down) != 0);
pr_info("TC2 power management initialized\n");
}
return ret;

Of course it is not because I'm helping you sidestepping firmware 
problems that I'll stop shaming those who came up with that firmware 
design.  ;-)


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Abhilash Kesavan]

Hi Nicolas,

The first man of the incoming cluster enables its snoops via the
power_up_setup function. During secondary boot-up, this does not occur
for the boot cluster. Hence, I enable the snoops for the boot cluster
as a one-time setup from the u-boot prompt. After secondary boot-up
there is no modification that I do. Where should this be ideally done
?

Regards,
Abhilash

On Sat, Jun 7, 2014 at 3:18 AM, Nicolas Pitre  wrote:
> On Fri, 6 Jun 2014, Abhilash Kesavan wrote:
>
>> Hi Doug,
>>
>> The first change in the kernel (clearing an iRAM location) is needed
>> because of an unnecessary change that we are carrying in the Chrome
>> U-boot. There is no reason for us to have the workaround in the
>> mainline kernel. Rather, we should remove the check from our u-boot.
>> However AFAIR a clean-up patch that I had posted internally was not
>> accepted as we had frozen the SPL at the time.
>>
>> The second change is to enable snoops for boot cluster. Internally, we
>> were disabling the snoops for both the clusters at power off and
>> enabling it in power_up_setup and power_up. However, I dropped the
>> approach due to problems pointed out by Nicolas here
>> http://www.spinics.net/lists/arm-kernel/msg324091.html related to
>> cpuidle. Hence, we turn it on at the u-boot.
>
> You should never *ever* play with the CCI from U-Boot.  That's for the
> MCPM layer to decide when it is safe to do so.  That's mainly the
> _whole_ reason why MCPM exists in the first place.
>
>
> Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Doug Anderson]

Nicolas,

On Fri, Jun 6, 2014 at 3:38 PM, Nicolas Pitre  wrote:
> On Fri, 6 Jun 2014, Doug Anderson wrote:
>
>> Note that handling CPU resume in a way that can be updated by RW
>> firmware is non-trivial and requires some SRAM to be saved across
>> suspend/resume.
>
> Saved by the kernel or the firmware?

By the SoC / board.

The problem on the original ARM Chromebook:

1. After resume, the boot CPU comes up just as if it were booting from
a cold boot.  All clocks are back at their default and the SDRAM
controller is no longer initted.  SDRAM is sitting in self refresh.

2. CPU loads boot code from SPI flash.

3. Boot code needs to be read only for security reasons.  You can have
RW updates for some portion of boot code, but only the part _after_
clock init (needed for SDRAM), SDRAM reinit, cache init.  That's
because you need those parts to (quickly) run crypto verification
code.


If the SoC / board can keep its internal SRAM powered across suspend
resume you've got an out.  Now you can have the firmware stash its
"wakeup" code into a location in SRAM.  At wakeup time you can jump
straight there and that code can worry about everything.  Now a
read-write firmware update can update the wakeup code and you're all
set.  In this case wakeup code doesn't survive a full power cycle /
reboot so you don't need any extra verification like you do if you
load from persistent storage.


That all still means you need to qualify a rw firmware update, but
that's better than nothing.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Fri, 6 Jun 2014, Doug Anderson wrote:

> Note that handling CPU resume in a way that can be updated by RW
> firmware is non-trivial and requires some SRAM to be saved across
> suspend/resume.

Saved by the kernel or the firmware?


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Fri, 6 Jun 2014, Olof Johansson wrote:

> On Fri, Jun 06, 2014 at 05:34:21PM -0400, Nicolas Pitre wrote:
> > On Fri, 6 Jun 2014, Olof Johansson wrote:
> > 
> > > On Sat, Jun 07, 2014 at 02:16:27AM +0530, Abhilash Kesavan wrote:
> > > > My answer is not "use mainline u-boot" primarily because I am not sure
> > > > mainline u-boot actually works on 5420 :).
> > > 
> > > And I'm saying that's not the answer primarily because we should never 
> > > require
> > > people to update their firmware to get a usable linux system.
> > > 
> > > > My answer is keep a patch
> > > > locally (or make a trivial change to the bootcmd) for people who would
> > > > like to use an upstream kernel with the firmware on the device. Once
> > > > we do have a working mainline u-boot, that can then be used by the
> > > > interested parties.
> > > 
> > > And I am strongly NAK:ing both of those approaches. We should not require
> > > a single out-of-tree patch because that means we have failed to make a 
> > > useful
> > > kernel for people. And it should never, ever, be a requirement for people 
> > > to
> > > reflash and risk bricking their device just to run mainline linux on it.
> > 
> > What we can do, though, is to publicly shame you all Google People very 
> > strongly for not making firmware updates in the field safer and easier.  
> > After all you must all know already that, by definition, software always 
> > contains bugs.  The first feature to be tested with a new 
> > bootloader/firmware must be the ability to successfully and safely (and 
> > securely) update itself.  Especially for a mainstream device like a 
> > Chromebook.
> 
> The security model of Chrome OS is that it relies on a read-only firmware
> that is later verifying and launching a read-write firwmare.

It must do a whole lot more than that, otherwise we wouldn't be talking 
about ways to update it.

> Some changes are
> not possible to work around in the read-write part of the firmware,
> or for some other reason becomes unfeasible to handle there.

In other words, you are pleading guilty for bad design.

> We're working on making the RO portion smaller, so we'll be less exposed
> to this in the future, but that's a feature that will take some time
> to mature. It can also be hard to motivate updating the firmware in
> the field for upstream usage, since doing these updates are a large
> undertaking from a QA point of view.

Usually, when the mechanism is there, it gets happily used even before 
mainline usage is considered.

> We're working hard on making sure a vanilla upstream kernel will work
> well on this generation of ARM Chromebooks, something we never really
> did with the first generation. If the response we get is "just carry it
> out of tree", why would we even care about upstream at all? We're trying
> to do the right thing here, even if it might require a line of code or
> two here or there that isn't perfect.

Shit happens.  One line or two is tolerable.

What is not acceptable is the impossibility to fix fundamental 
correctness issues because people think their damn read-only firmware is 
bug free.  I've spent a great amount of time doing multiple rounds of 
reviews for MCPM backends.  Still it seems that people just don't get 
all the correctness subtleties before at least 5 rounds.  Yet they 
thought their code was OK on the initial rounds, otherwise why would 
they knowingly post buggy code?  If that code had been put into 
read-only firmware, say behind some PSCI interface for example, then 
your device would simply be stuck to board-bringup feature level.

I don't know enough about the actual issue with the Chromebook yet, but 
I do hope your firmware screwups aren't too serious.  The patch Doug 
just posted appears innocent enough.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Doug Anderson]

Hi,

To add a few things to Olof's comments:

On Fri, Jun 6, 2014 at 2:49 PM, Olof Johansson  wrote:
>> What we can do, though, is to publicly shame you all Google People very
>> strongly for not making firmware updates in the field safer and easier.
>> After all you must all know already that, by definition, software always
>> contains bugs.  The first feature to be tested with a new
>> bootloader/firmware must be the ability to successfully and safely (and
>> securely) update itself.  Especially for a mainstream device like a
>> Chromebook.
>
> The security model of Chrome OS is that it relies on a read-only firmware
> that is later verifying and launching a read-write firwmare. Obviously
> changes to the read-only firwmare is impossible in the field (and only
> done at risk of bricking the device for hobbyists). Some changes are
> not possible to work around in the read-write part of the firmware,
> or for some other reason becomes unfeasible to handle there.
>
> We're working on making the RO portion smaller, so we'll be less exposed
> to this in the future, but that's a feature that will take some time
> to mature.

One thing to be aware is that despite the fact that these machines
just came out, their firmware has been roughly frozen (no major
changes) since last September due to various (non-technical) issues.
We almost got the massive rewrite in before the deadline (talk to
Simon Glass about this) but didn't quite make the deadline.  :(

Note that handling CPU resume in a way that can be updated by RW
firmware is non-trivial and requires some SRAM to be saved across
suspend/resume.  On the original Samsung Chromebook we didn't have
that.  On the Samsung Chromebook 2 we do (yay!) but we didn't realize
it until pretty late in the game.  Sigh.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Olof Johansson]

On Fri, Jun 06, 2014 at 05:34:21PM -0400, Nicolas Pitre wrote:
> On Fri, 6 Jun 2014, Olof Johansson wrote:
> 
> > On Sat, Jun 07, 2014 at 02:16:27AM +0530, Abhilash Kesavan wrote:
> > > My answer is not "use mainline u-boot" primarily because I am not sure
> > > mainline u-boot actually works on 5420 :).
> > 
> > And I'm saying that's not the answer primarily because we should never 
> > require
> > people to update their firmware to get a usable linux system.
> > 
> > > My answer is keep a patch
> > > locally (or make a trivial change to the bootcmd) for people who would
> > > like to use an upstream kernel with the firmware on the device. Once
> > > we do have a working mainline u-boot, that can then be used by the
> > > interested parties.
> > 
> > And I am strongly NAK:ing both of those approaches. We should not require
> > a single out-of-tree patch because that means we have failed to make a 
> > useful
> > kernel for people. And it should never, ever, be a requirement for people to
> > reflash and risk bricking their device just to run mainline linux on it.
> 
> What we can do, though, is to publicly shame you all Google People very 
> strongly for not making firmware updates in the field safer and easier.  
> After all you must all know already that, by definition, software always 
> contains bugs.  The first feature to be tested with a new 
> bootloader/firmware must be the ability to successfully and safely (and 
> securely) update itself.  Especially for a mainstream device like a 
> Chromebook.

The security model of Chrome OS is that it relies on a read-only firmware
that is later verifying and launching a read-write firwmare. Obviously
changes to the read-only firwmare is impossible in the field (and only
done at risk of bricking the device for hobbyists). Some changes are
not possible to work around in the read-write part of the firmware,
or for some other reason becomes unfeasible to handle there.

We're working on making the RO portion smaller, so we'll be less exposed
to this in the future, but that's a feature that will take some time
to mature. It can also be hard to motivate updating the firmware in
the field for upstream usage, since doing these updates are a large
undertaking from a QA point of view. When we can bundle them with other
high-priority changes (without adding significant risk), we try to do so.

Anyway, with that being said: The real reason we're in this situation
at this time is that the upstream review and discussion (including
patch posting) didn't happen until after RO firmware had been cut, and
some pieces were not acceptable (things that we had not realized
in our internal reviews when we were working on the product). If SoC
vendors upstream code earlier, we can be more certain that the upstream
implementation will work well with the firmware we have. That's the
_real_ solution to this situation. We're working with vendors to make
them understand this (work closer and earlier with upstream). Samsung is
making a great effort on 5420/5800, but things won't be perfect overnight.

> > It's an artificial barrier of entry with high risk, and we'll be worse 
> > off for adding it. Same for out-of-tree patches.
> 
> So ... let's find the lesser of all evils ... as always.

We're working hard on making sure a vanilla upstream kernel will work
well on this generation of ARM Chromebooks, something we never really
did with the first generation. If the response we get is "just carry it
out of tree", why would we even care about upstream at all? We're trying
to do the right thing here, even if it might require a line of code or
two here or there that isn't perfect.

> > iRAM is covered on Doug's sub-thread, and I think his approach looks 
> > promising.
> > So, it seems like we have a solution both to enable the CCI port and to 
> > avoid
> > clearing iram -- we should be set?
> 
> Care to resume the proposed solution then?

Doug has posted patches for the IRAM piece, and Andrew was going to look
at the MCPM piece. So that's already happening.


-Olof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Fri, 6 Jun 2014, Abhilash Kesavan wrote:

> Hi Doug,
> 
> The first change in the kernel (clearing an iRAM location) is needed
> because of an unnecessary change that we are carrying in the Chrome
> U-boot. There is no reason for us to have the workaround in the
> mainline kernel. Rather, we should remove the check from our u-boot.
> However AFAIR a clean-up patch that I had posted internally was not
> accepted as we had frozen the SPL at the time.
> 
> The second change is to enable snoops for boot cluster. Internally, we
> were disabling the snoops for both the clusters at power off and
> enabling it in power_up_setup and power_up. However, I dropped the
> approach due to problems pointed out by Nicolas here
> http://www.spinics.net/lists/arm-kernel/msg324091.html related to
> cpuidle. Hence, we turn it on at the u-boot.

You should never *ever* play with the CCI from U-Boot.  That's for the 
MCPM layer to decide when it is safe to do so.  That's mainly the 
_whole_ reason why MCPM exists in the first place.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Doug Anderson]

Hi,

On Fri, Jun 6, 2014 at 1:49 PM, Doug Anderson  wrote:
>> Are you talking about the re-writing the mcpm entry point address 
>> post-resume ?
>
> Or even (as Andrew points out) just don't use it.
>
> This works and IMHO is much cleaner because it totally removes the
> U-Boot dependency.  I'll cleanup to not be so insane and post:

The less insane version is at
.  Thanks to Andrew for
the suggestion!

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Nicolas Pitre]

On Fri, 6 Jun 2014, Olof Johansson wrote:

> On Sat, Jun 07, 2014 at 02:16:27AM +0530, Abhilash Kesavan wrote:
> > My answer is not "use mainline u-boot" primarily because I am not sure
> > mainline u-boot actually works on 5420 :).
> 
> And I'm saying that's not the answer primarily because we should never require
> people to update their firmware to get a usable linux system.
> 
> > My answer is keep a patch
> > locally (or make a trivial change to the bootcmd) for people who would
> > like to use an upstream kernel with the firmware on the device. Once
> > we do have a working mainline u-boot, that can then be used by the
> > interested parties.
> 
> And I am strongly NAK:ing both of those approaches. We should not require
> a single out-of-tree patch because that means we have failed to make a useful
> kernel for people. And it should never, ever, be a requirement for people to
> reflash and risk bricking their device just to run mainline linux on it.

What we can do, though, is to publicly shame you all Google People very 
strongly for not making firmware updates in the field safer and easier.  
After all you must all know already that, by definition, software always 
contains bugs.  The first feature to be tested with a new 
bootloader/firmware must be the ability to successfully and safely (and 
securely) update itself.  Especially for a mainstream device like a 
Chromebook.

> It's an artificial barrier of entry with high risk, and we'll be worse 
> off for adding it. Same for out-of-tree patches.

So ... let's find the lesser of all evils ... as always.

> iRAM is covered on Doug's sub-thread, and I think his approach looks 
> promising.
> So, it seems like we have a solution both to enable the CCI port and to avoid
> clearing iram -- we should be set?

Care to resume the proposed solution then?


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Doug Anderson]

Hi,

On Fri, Jun 6, 2014 at 2:01 PM, Russell King - ARM Linux
 wrote:
> On Fri, Jun 06, 2014 at 01:49:11PM -0700, Doug Anderson wrote:
>> This works and IMHO is much cleaner because it totally removes the
>> U-Boot dependency.  I'll cleanup to not be so insane and post:
>>
>> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c
>> b/arch/arm/mach-exynos/mcpm-exynos.c
>> index 0498d0b..9c5df7b 100644
>> --- a/arch/arm/mach-exynos/mcpm-exynos.c
>> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
>> @@ -290,6 +290,14 @@ static void __naked
>> exynos_pm_power_up_setup(unsigned int affinity_level)
>> "b  cci_enable_port_for_self");
>>  }
>>
>> +static void __naked exynos_mcpm_secondary_cpu_start(void)
>> +{
>> +   asm volatile ("\n"
>> +   "ldrr0, [pc, #0]\n"
>> +   "bx r0\n"
>> +   ".word  0" );
>> +}
>> +
>
> So does it matter whether the above code gets assembled as thumb or
> ARM?  How does your caller know which ISA mode to enter this fragment
> in?

I'm going to take Olof's suggestion and just hardcode the instructions like:

__raw_writel(0xe59f, ns_sram_base_addr); /* ldr r0, [pc, #0] */
__raw_writel(0xe12fff10, ns_sram_base_addr + 4); /* bx  r0 */
__raw_writel(virt_to_phys(mcpm_entry_point), ns_sram_base_addr + 8);

The caller always jumps to this code with "bx" but always jumps into
ARM mode.  Specifically U-Boot will have:

  branch_bx(CONFIG_EXYNOS_RELOCATE_CODE_BASE);

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Abhilash Kesavan]

Hi Olof,

On Sat, Jun 7, 2014 at 2:31 AM, Olof Johansson  wrote:
> On Sat, Jun 07, 2014 at 02:16:27AM +0530, Abhilash Kesavan wrote:
>> Hi Olof,
>>
>> On Sat, Jun 7, 2014 at 2:07 AM, Olof Johansson  wrote:
>> > [Adding Nico since he was involved in the original reviews]
>> >
>> > Hi,
>> >
>> > On Fri, Jun 06, 2014 at 11:20:56AM -0700, Doug Anderson wrote:
>> >> Abhilash,
>> >>
>> >>
>> >>
>> >> On Fri, Jun 6, 2014 at 11:12 AM, Abhilash Kesavan
>> >>  wrote:
>> >> > Hi Doug,
>> >> >
>> >> > On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  
>> >> > wrote:
>> >> >> Abhilash,
>> >> >>
>> >> >> On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
>> >> >>  wrote:
>> >> >>> Hi Doug,
>> >> >>>
>> >> >>> The first change in the kernel (clearing an iRAM location) is needed
>> >> >>> because of an unnecessary change that we are carrying in the Chrome
>> >> >>> U-boot. There is no reason for us to have the workaround in the
>> >> >>> mainline kernel. Rather, we should remove the check from our u-boot.
>> >> >>> However AFAIR a clean-up patch that I had posted internally was not
>> >> >>> accepted as we had frozen the SPL at the time.
>> >> >>
>> >> >> Ah, is that this one, or a different one?
>> >> >>
>> >> >> https://chromium-review.googlesource.com/#/c/66049/
>> >> > Yes, this along with a kernel side change.
>> >>
>> >> Can we safely take this one without the kernel-side one?
>> >>
>> >>
>> >> >> If we land that patch now it won't help since nobody is going to be
>> >> >> updating their read-only firmware.  We'll need to put code somewhere
>> >> >> that fixes it.
>> >> > We just carry the workaround fix locally until we migrate to mainline
>> >> > u-boot for 5420 where the unnecessay check will not be present.
>> >>
>> >> I think there are people out there who want to run a mainline kernel
>> >> on existing Chromebook 2 hardware and don't want to rewrite their RO
>> >> firmware.  We need a solution for those people.
>> >
>> > Agree. The answer to this is most definitely _not_ "install mainline 
>> > u-boot".
>> > The upstream kernel needs to be able to boot with the firmware that was 
>> > shipped
>> > on the device.
>>
>> My answer is not "use mainline u-boot" primarily because I am not sure
>> mainline u-boot actually works on 5420 :).
>
> And I'm saying that's not the answer primarily because we should never require
> people to update their firmware to get a usable linux system.
>
>> My answer is keep a patch
>> locally (or make a trivial change to the bootcmd) for people who would
>> like to use an upstream kernel with the firmware on the device. Once
>> we do have a working mainline u-boot, that can then be used by the
>> interested parties.
>
> And I am strongly NAK:ing both of those approaches. We should not require
> a single out-of-tree patch because that means we have failed to make a useful
> kernel for people. And it should never, ever, be a requirement for people to
> reflash and risk bricking their device just to run mainline linux on it. It's
> an artificial barrier of entry with high risk, and we'll be worse off for
> adding it. Same for out-of-tree patches.
I have explained my reasons in the thread. I continue to believe that
we should not be adding code in the kernel that is specifically
handling an oversight that exists in the Chrome U-boot. However, since
you have NAK'ed my approaches, we are left with Doug's.
>
>> > In this case it shouldn't be controversial to add this. What we need is
>> > a one-time boot-time setup, not runtime so cpuidle shouldn't be a factor
>> > at that time. The earlier reservations were about runtime changes and this 
>> > is
>> > quite different.
>> I think there is some confusion here, the clearing of the iRAM
>> location is what I have been pushing against. It has got nothing to do
>> cpuidle. If it were to be done then  it would be a one time setup and
>> I could quite easily do it in mcpm_init.
>
> iRAM is covered on Doug's sub-thread, and I think his approach looks 
> promising.
> So, it seems like we have a solution both to enable the CCI port and to avoid
> clearing iram -- we should be set?
I'll have a look at Doug's updated patches tomorrow or on Monday.

Regards,
Abhilash
>
>
> -Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Russell King - ARM Linux]

On Fri, Jun 06, 2014 at 01:49:11PM -0700, Doug Anderson wrote:
> This works and IMHO is much cleaner because it totally removes the
> U-Boot dependency.  I'll cleanup to not be so insane and post:
> 
> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c
> b/arch/arm/mach-exynos/mcpm-exynos.c
> index 0498d0b..9c5df7b 100644
> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> @@ -290,6 +290,14 @@ static void __naked
> exynos_pm_power_up_setup(unsigned int affinity_level)
> "b  cci_enable_port_for_self");
>  }
> 
> +static void __naked exynos_mcpm_secondary_cpu_start(void)
> +{
> +   asm volatile ("\n"
> +   "ldrr0, [pc, #0]\n"
> +   "bx r0\n"
> +   ".word  0" );
> +}
> +

So does it matter whether the above code gets assembled as thumb or
ARM?  How does your caller know which ISA mode to enter this fragment
in?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Olof Johansson]

On Sat, Jun 07, 2014 at 02:16:27AM +0530, Abhilash Kesavan wrote:
> Hi Olof,
> 
> On Sat, Jun 7, 2014 at 2:07 AM, Olof Johansson  wrote:
> > [Adding Nico since he was involved in the original reviews]
> >
> > Hi,
> >
> > On Fri, Jun 06, 2014 at 11:20:56AM -0700, Doug Anderson wrote:
> >> Abhilash,
> >>
> >>
> >>
> >> On Fri, Jun 6, 2014 at 11:12 AM, Abhilash Kesavan
> >>  wrote:
> >> > Hi Doug,
> >> >
> >> > On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  
> >> > wrote:
> >> >> Abhilash,
> >> >>
> >> >> On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
> >> >>  wrote:
> >> >>> Hi Doug,
> >> >>>
> >> >>> The first change in the kernel (clearing an iRAM location) is needed
> >> >>> because of an unnecessary change that we are carrying in the Chrome
> >> >>> U-boot. There is no reason for us to have the workaround in the
> >> >>> mainline kernel. Rather, we should remove the check from our u-boot.
> >> >>> However AFAIR a clean-up patch that I had posted internally was not
> >> >>> accepted as we had frozen the SPL at the time.
> >> >>
> >> >> Ah, is that this one, or a different one?
> >> >>
> >> >> https://chromium-review.googlesource.com/#/c/66049/
> >> > Yes, this along with a kernel side change.
> >>
> >> Can we safely take this one without the kernel-side one?
> >>
> >>
> >> >> If we land that patch now it won't help since nobody is going to be
> >> >> updating their read-only firmware.  We'll need to put code somewhere
> >> >> that fixes it.
> >> > We just carry the workaround fix locally until we migrate to mainline
> >> > u-boot for 5420 where the unnecessay check will not be present.
> >>
> >> I think there are people out there who want to run a mainline kernel
> >> on existing Chromebook 2 hardware and don't want to rewrite their RO
> >> firmware.  We need a solution for those people.
> >
> > Agree. The answer to this is most definitely _not_ "install mainline 
> > u-boot".
> > The upstream kernel needs to be able to boot with the firmware that was 
> > shipped
> > on the device.
>
> My answer is not "use mainline u-boot" primarily because I am not sure
> mainline u-boot actually works on 5420 :).

And I'm saying that's not the answer primarily because we should never require
people to update their firmware to get a usable linux system.

> My answer is keep a patch
> locally (or make a trivial change to the bootcmd) for people who would
> like to use an upstream kernel with the firmware on the device. Once
> we do have a working mainline u-boot, that can then be used by the
> interested parties.

And I am strongly NAK:ing both of those approaches. We should not require
a single out-of-tree patch because that means we have failed to make a useful
kernel for people. And it should never, ever, be a requirement for people to
reflash and risk bricking their device just to run mainline linux on it. It's
an artificial barrier of entry with high risk, and we'll be worse off for
adding it. Same for out-of-tree patches.

> > In this case it shouldn't be controversial to add this. What we need is
> > a one-time boot-time setup, not runtime so cpuidle shouldn't be a factor
> > at that time. The earlier reservations were about runtime changes and this 
> > is
> > quite different.
> I think there is some confusion here, the clearing of the iRAM
> location is what I have been pushing against. It has got nothing to do
> cpuidle. If it were to be done then  it would be a one time setup and
> I could quite easily do it in mcpm_init.

iRAM is covered on Doug's sub-thread, and I think his approach looks promising.
So, it seems like we have a solution both to enable the CCI port and to avoid
clearing iram -- we should be set?


-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Doug Anderson]

On Fri, Jun 6, 2014 at 12:09 PM, Abhilash Kesavan
 wrote:
> Hi Doug,
>
> On Sat, Jun 7, 2014 at 12:26 AM, Doug Anderson  wrote:
>> Abhilash,
>>
>> On Fri, Jun 6, 2014 at 11:31 AM, Abhilash Kesavan
>>  wrote:
>>> Hi Doug,
>>>
>>> On Fri, Jun 6, 2014 at 11:50 PM, Doug Anderson  wrote:
 Abhilash,



 On Fri, Jun 6, 2014 at 11:12 AM, Abhilash Kesavan
  wrote:
> Hi Doug,
>
> On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  
> wrote:
>> Abhilash,
>>
>> On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
>>  wrote:
>>> Hi Doug,
>>>
>>> The first change in the kernel (clearing an iRAM location) is needed
>>> because of an unnecessary change that we are carrying in the Chrome
>>> U-boot. There is no reason for us to have the workaround in the
>>> mainline kernel. Rather, we should remove the check from our u-boot.
>>> However AFAIR a clean-up patch that I had posted internally was not
>>> accepted as we had frozen the SPL at the time.
>>
>> Ah, is that this one, or a different one?
>>
>> https://chromium-review.googlesource.com/#/c/66049/
> Yes, this along with a kernel side change.

 Can we safely take this one without the kernel-side one?
>>> Yes, just the u-boot change should suffice.


>> If we land that patch now it won't help since nobody is going to be
>> updating their read-only firmware.  We'll need to put code somewhere
>> that fixes it.
> We just carry the workaround fix locally until we migrate to mainline
> u-boot for 5420 where the unnecessay check will not be present.

 I think there are people out there who want to run a mainline kernel
 on existing Chromebook 2 hardware and don't want to rewrite their RO
 firmware.  We need a solution for those people.
>>> Yes, I see your point. But, do you think someone who has changed the
>>> existing fused kernel on the device to a mainline one would be averse
>>> to applying a couple of small work-around changes as well ? Their
>>> finding this thread and the proposed "magic" fixes may be difficult
>>> but not the actual application I think.
>>>
>>> How about having a page similar to
>>> "http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/using-an-upstream-kernel-on-snow";
>>> for Peach ? We could have the work-arounds listed there.
>>
>> We can (though the fewer weird things we have the better), but we
>> definitely need to provide workarounds that don't require people to
>> change their RO firmware.
> I do not quite agree with this. They do not need to change their RO
> firmware, just modify their boot commands.
>>
>> Thinking all that through, I think the answer is that we want to
>> abandon the U-Boot change above
>> .  At this point
>> we're never going to take it at this point and there's no possible way
>> to do it in an RW firmware update (right?) since any workaround would
>> be overwritten by the SPL at resume time.
> Sure, will abandon.
>>
>> So the proper "fix" for the "mw.l 02073028 0" is a kernel fix.  ...and
> I believe there is no "proper" fix for incorrect existing code in
> non-mainline u-boot.
>
>> if upstream doesn't want land it because it's impure then we'll just
>> have to list it on our "apply these hacks to your kernel".  You sent
>> me this as a kernel fix before and now I think I understand why (to
>> handle the s2r case).  Can you please post this up to the mailing
>> list?  Please make sure it will handle the suspend/resume case
>> whenever suspend/resume starts working (I haven't tested but I
>> remember hearing that it doesn't work).
>
> Are you talking about the re-writing the mcpm entry point address post-resume 
> ?

Or even (as Andrew points out) just don't use it.

This works and IMHO is much cleaner because it totally removes the
U-Boot dependency.  I'll cleanup to not be so insane and post:

diff --git a/arch/arm/mach-exynos/mcpm-exynos.c
b/arch/arm/mach-exynos/mcpm-exynos.c
index 0498d0b..9c5df7b 100644
--- a/arch/arm/mach-exynos/mcpm-exynos.c
+++ b/arch/arm/mach-exynos/mcpm-exynos.c
@@ -290,6 +290,14 @@ static void __naked
exynos_pm_power_up_setup(unsigned int affinity_level)
"b  cci_enable_port_for_self");
 }

+static void __naked exynos_mcpm_secondary_cpu_start(void)
+{
+   asm volatile ("\n"
+   "ldrr0, [pc, #0]\n"
+   "bx r0\n"
+   ".word  0" );
+}
+
 static const struct of_device_id exynos_dt_mcpm_match[] = {
{ .compatible = "samsung,exynos5420" },
{ .compatible = "samsung,exynos5800" },
@@ -346,8 +354,9 @@ static int __init exynos_mcpm_init(void)
 * Future entries into the kernel can now go
 * through the cluster entry vectors.
 */
-   __raw_writel(virt_to_phys(mcpm_entry_point),
-   ns_sram_base_addr + MCPM_BOOT_ADDR_OFFSET);
+   __raw_writel(((u32*)exynos_mcpm_secondary_cpu_start)[0],
ns_sram_

Re: Problems booting exynos5420 with >1 CPU

[Abhilash Kesavan]

Hi Olof,

On Sat, Jun 7, 2014 at 2:07 AM, Olof Johansson  wrote:
> [Adding Nico since he was involved in the original reviews]
>
> Hi,
>
> On Fri, Jun 06, 2014 at 11:20:56AM -0700, Doug Anderson wrote:
>> Abhilash,
>>
>>
>>
>> On Fri, Jun 6, 2014 at 11:12 AM, Abhilash Kesavan
>>  wrote:
>> > Hi Doug,
>> >
>> > On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  wrote:
>> >> Abhilash,
>> >>
>> >> On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
>> >>  wrote:
>> >>> Hi Doug,
>> >>>
>> >>> The first change in the kernel (clearing an iRAM location) is needed
>> >>> because of an unnecessary change that we are carrying in the Chrome
>> >>> U-boot. There is no reason for us to have the workaround in the
>> >>> mainline kernel. Rather, we should remove the check from our u-boot.
>> >>> However AFAIR a clean-up patch that I had posted internally was not
>> >>> accepted as we had frozen the SPL at the time.
>> >>
>> >> Ah, is that this one, or a different one?
>> >>
>> >> https://chromium-review.googlesource.com/#/c/66049/
>> > Yes, this along with a kernel side change.
>>
>> Can we safely take this one without the kernel-side one?
>>
>>
>> >> If we land that patch now it won't help since nobody is going to be
>> >> updating their read-only firmware.  We'll need to put code somewhere
>> >> that fixes it.
>> > We just carry the workaround fix locally until we migrate to mainline
>> > u-boot for 5420 where the unnecessay check will not be present.
>>
>> I think there are people out there who want to run a mainline kernel
>> on existing Chromebook 2 hardware and don't want to rewrite their RO
>> firmware.  We need a solution for those people.
>
> Agree. The answer to this is most definitely _not_ "install mainline u-boot".
> The upstream kernel needs to be able to boot with the firmware that was 
> shipped
> on the device.
My answer is not "use mainline u-boot" primarily because I am not sure
mainline u-boot actually works on 5420 :). My answer is keep a patch
locally (or make a trivial change to the bootcmd) for people who would
like to use an upstream kernel with the firmware on the device. Once
we do have a working mainline u-boot, that can then be used by the
interested parties.
>
> In this case it shouldn't be controversial to add this. What we need is
> a one-time boot-time setup, not runtime so cpuidle shouldn't be a factor
> at that time. The earlier reservations were about runtime changes and this is
> quite different.
I think there is some confusion here, the clearing of the iRAM
location is what I have been pushing against. It has got nothing to do
cpuidle. If it were to be done then  it would be a one time setup and
I could quite easily do it in mcpm_init.

Regards,
Abhilash
>
>> >>> The second change is to enable snoops for boot cluster. Internally, we
>> >>> were disabling the snoops for both the clusters at power off and
>> >>> enabling it in power_up_setup and power_up. However, I dropped the
>> >>> approach due to problems pointed out by Nicolas here
>> >>> http://www.spinics.net/lists/arm-kernel/msg324091.html related to
>> >>> cpuidle. Hence, we turn it on at the u-boot.
>> >>
>> >> I think I followed all that.  What you're saying is that our kernel
>> >> dynamically enables and disables snoops as needed, but Nicolas pointed
>> >> out that it was unsafe (though apparently we're not seeing problems in
>> >> our usage).
>> > We did not see any problems as CPUIdle was one of the problematic
>> > scenarios which we have not got enabled.
>>
>> Ah, makes sense!
>>
>>
>>
>> I'm still trying to figure out all of this code, but we'll also need
>> to make sure whatever solution we come up with handles suspend/resume
>> properly.  I know SRAM is lost across suspend/resume so someone
>> (either the SPL from read-only memory or the kernel) must be
>> recreating the SRAM structures after S2R...
>
>
> -Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Olof Johansson]

[Adding Nico since he was involved in the original reviews]

Hi,

On Fri, Jun 06, 2014 at 11:20:56AM -0700, Doug Anderson wrote:
> Abhilash,
> 
> 
> 
> On Fri, Jun 6, 2014 at 11:12 AM, Abhilash Kesavan
>  wrote:
> > Hi Doug,
> >
> > On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  wrote:
> >> Abhilash,
> >>
> >> On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
> >>  wrote:
> >>> Hi Doug,
> >>>
> >>> The first change in the kernel (clearing an iRAM location) is needed
> >>> because of an unnecessary change that we are carrying in the Chrome
> >>> U-boot. There is no reason for us to have the workaround in the
> >>> mainline kernel. Rather, we should remove the check from our u-boot.
> >>> However AFAIR a clean-up patch that I had posted internally was not
> >>> accepted as we had frozen the SPL at the time.
> >>
> >> Ah, is that this one, or a different one?
> >>
> >> https://chromium-review.googlesource.com/#/c/66049/
> > Yes, this along with a kernel side change.
> 
> Can we safely take this one without the kernel-side one?
> 
> 
> >> If we land that patch now it won't help since nobody is going to be
> >> updating their read-only firmware.  We'll need to put code somewhere
> >> that fixes it.
> > We just carry the workaround fix locally until we migrate to mainline
> > u-boot for 5420 where the unnecessay check will not be present.
> 
> I think there are people out there who want to run a mainline kernel
> on existing Chromebook 2 hardware and don't want to rewrite their RO
> firmware.  We need a solution for those people.

Agree. The answer to this is most definitely _not_ "install mainline u-boot".
The upstream kernel needs to be able to boot with the firmware that was shipped
on the device.

In this case it shouldn't be controversial to add this. What we need is
a one-time boot-time setup, not runtime so cpuidle shouldn't be a factor
at that time. The earlier reservations were about runtime changes and this is
quite different.

> >>> The second change is to enable snoops for boot cluster. Internally, we
> >>> were disabling the snoops for both the clusters at power off and
> >>> enabling it in power_up_setup and power_up. However, I dropped the
> >>> approach due to problems pointed out by Nicolas here
> >>> http://www.spinics.net/lists/arm-kernel/msg324091.html related to
> >>> cpuidle. Hence, we turn it on at the u-boot.
> >>
> >> I think I followed all that.  What you're saying is that our kernel
> >> dynamically enables and disables snoops as needed, but Nicolas pointed
> >> out that it was unsafe (though apparently we're not seeing problems in
> >> our usage).
> > We did not see any problems as CPUIdle was one of the problematic
> > scenarios which we have not got enabled.
> 
> Ah, makes sense!
> 
> 
> 
> I'm still trying to figure out all of this code, but we'll also need
> to make sure whatever solution we come up with handles suspend/resume
> properly.  I know SRAM is lost across suspend/resume so someone
> (either the SPL from read-only memory or the kernel) must be
> recreating the SRAM structures after S2R...


-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Abhilash Kesavan]

On Sat, Jun 7, 2014 at 12:39 AM, Abhilash Kesavan
 wrote:
> Hi Doug,
>
> On Sat, Jun 7, 2014 at 12:26 AM, Doug Anderson  wrote:
>> Abhilash,
>>
>> On Fri, Jun 6, 2014 at 11:31 AM, Abhilash Kesavan
>>  wrote:
>>> Hi Doug,
>>>
>>> On Fri, Jun 6, 2014 at 11:50 PM, Doug Anderson  wrote:
 Abhilash,



 On Fri, Jun 6, 2014 at 11:12 AM, Abhilash Kesavan
  wrote:
> Hi Doug,
>
> On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  
> wrote:
>> Abhilash,
>>
>> On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
>>  wrote:
>>> Hi Doug,
>>>
>>> The first change in the kernel (clearing an iRAM location) is needed
>>> because of an unnecessary change that we are carrying in the Chrome
>>> U-boot. There is no reason for us to have the workaround in the
>>> mainline kernel. Rather, we should remove the check from our u-boot.
>>> However AFAIR a clean-up patch that I had posted internally was not
>>> accepted as we had frozen the SPL at the time.
>>
>> Ah, is that this one, or a different one?
>>
>> https://chromium-review.googlesource.com/#/c/66049/
> Yes, this along with a kernel side change.

 Can we safely take this one without the kernel-side one?
>>> Yes, just the u-boot change should suffice.


>> If we land that patch now it won't help since nobody is going to be
>> updating their read-only firmware.  We'll need to put code somewhere
>> that fixes it.
> We just carry the workaround fix locally until we migrate to mainline
> u-boot for 5420 where the unnecessay check will not be present.

 I think there are people out there who want to run a mainline kernel
 on existing Chromebook 2 hardware and don't want to rewrite their RO
 firmware.  We need a solution for those people.
>>> Yes, I see your point. But, do you think someone who has changed the
>>> existing fused kernel on the device to a mainline one would be averse
>>> to applying a couple of small work-around changes as well ? Their
>>> finding this thread and the proposed "magic" fixes may be difficult
>>> but not the actual application I think.
>>>
>>> How about having a page similar to
>>> "http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/using-an-upstream-kernel-on-snow";
>>> for Peach ? We could have the work-arounds listed there.
>>
>> We can (though the fewer weird things we have the better), but we
>> definitely need to provide workarounds that don't require people to
>> change their RO firmware.
> I do not quite agree with this. They do not need to change their RO
> firmware, just modify their boot commands.
>>
>> Thinking all that through, I think the answer is that we want to
>> abandon the U-Boot change above
>> .  At this point
>> we're never going to take it at this point and there's no possible way
>> to do it in an RW firmware update (right?) since any workaround would
>> be overwritten by the SPL at resume time.
> Sure, will abandon.
>>
>> So the proper "fix" for the "mw.l 02073028 0" is a kernel fix.  ...and
> I believe there is no "proper" fix for incorrect existing code in
> non-mainline u-boot.
>
>> if upstream doesn't want land it because it's impure then we'll just
>> have to list it on our "apply these hacks to your kernel".  You sent
>> me this as a kernel fix before and now I think I understand why (to
>> handle the s2r case).  Can you please post this up to the mailing
>> list?  Please make sure it will handle the suspend/resume case
>> whenever suspend/resume starts working (I haven't tested but I
>> remember hearing that it doesn't work).
>
> Are you talking about the re-writing the mcpm entry point address post-resume 
> ?
S2R does not work right now, but patches have been posted for the
same. I did test the MCPM patches across an S2R cycle and it worked
fine. Implying the SRAM was being retained. I guess that would be
because the regulator support has not been added/enabled for Peach.
>>
>> Note: I think there are actually two possible kernel fixes (right?):
>> 1. Have the kernel itself (re)write the code at 0x02073000 at bootup /
>> resume time.  I don't know of any reason why this should need to be in
>> U-Boot.  Maybe upstream would take this?
>
>>
>> 2. Have the kernel clear this flag to work with existing Chromebook 2 
>> firmware.
>>
>> --
>>
>> The proper "fix" for the "mw.l 10d25000 3" is a U-Boot fix.  Let me
>> see if I can put together something that will handle this in RW
>> U-Boot.  Even if we don't officially ship this RW U-Boot we can always
>> point people at the binaries.
>>
>>
>> Does that make sense?
>>
>> -Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Abhilash Kesavan]

Hi Doug,

On Sat, Jun 7, 2014 at 12:26 AM, Doug Anderson  wrote:
> Abhilash,
>
> On Fri, Jun 6, 2014 at 11:31 AM, Abhilash Kesavan
>  wrote:
>> Hi Doug,
>>
>> On Fri, Jun 6, 2014 at 11:50 PM, Doug Anderson  wrote:
>>> Abhilash,
>>>
>>>
>>>
>>> On Fri, Jun 6, 2014 at 11:12 AM, Abhilash Kesavan
>>>  wrote:
 Hi Doug,

 On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  wrote:
> Abhilash,
>
> On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
>  wrote:
>> Hi Doug,
>>
>> The first change in the kernel (clearing an iRAM location) is needed
>> because of an unnecessary change that we are carrying in the Chrome
>> U-boot. There is no reason for us to have the workaround in the
>> mainline kernel. Rather, we should remove the check from our u-boot.
>> However AFAIR a clean-up patch that I had posted internally was not
>> accepted as we had frozen the SPL at the time.
>
> Ah, is that this one, or a different one?
>
> https://chromium-review.googlesource.com/#/c/66049/
 Yes, this along with a kernel side change.
>>>
>>> Can we safely take this one without the kernel-side one?
>> Yes, just the u-boot change should suffice.
>>>
>>>
> If we land that patch now it won't help since nobody is going to be
> updating their read-only firmware.  We'll need to put code somewhere
> that fixes it.
 We just carry the workaround fix locally until we migrate to mainline
 u-boot for 5420 where the unnecessay check will not be present.
>>>
>>> I think there are people out there who want to run a mainline kernel
>>> on existing Chromebook 2 hardware and don't want to rewrite their RO
>>> firmware.  We need a solution for those people.
>> Yes, I see your point. But, do you think someone who has changed the
>> existing fused kernel on the device to a mainline one would be averse
>> to applying a couple of small work-around changes as well ? Their
>> finding this thread and the proposed "magic" fixes may be difficult
>> but not the actual application I think.
>>
>> How about having a page similar to
>> "http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/using-an-upstream-kernel-on-snow";
>> for Peach ? We could have the work-arounds listed there.
>
> We can (though the fewer weird things we have the better), but we
> definitely need to provide workarounds that don't require people to
> change their RO firmware.
I do not quite agree with this. They do not need to change their RO
firmware, just modify their boot commands.
>
> Thinking all that through, I think the answer is that we want to
> abandon the U-Boot change above
> .  At this point
> we're never going to take it at this point and there's no possible way
> to do it in an RW firmware update (right?) since any workaround would
> be overwritten by the SPL at resume time.
Sure, will abandon.
>
> So the proper "fix" for the "mw.l 02073028 0" is a kernel fix.  ...and
I believe there is no "proper" fix for incorrect existing code in
non-mainline u-boot.

> if upstream doesn't want land it because it's impure then we'll just
> have to list it on our "apply these hacks to your kernel".  You sent
> me this as a kernel fix before and now I think I understand why (to
> handle the s2r case).  Can you please post this up to the mailing
> list?  Please make sure it will handle the suspend/resume case
> whenever suspend/resume starts working (I haven't tested but I
> remember hearing that it doesn't work).

Are you talking about the re-writing the mcpm entry point address post-resume ?
>
> Note: I think there are actually two possible kernel fixes (right?):
> 1. Have the kernel itself (re)write the code at 0x02073000 at bootup /
> resume time.  I don't know of any reason why this should need to be in
> U-Boot.  Maybe upstream would take this?

>
> 2. Have the kernel clear this flag to work with existing Chromebook 2 
> firmware.
>
> --
>
> The proper "fix" for the "mw.l 10d25000 3" is a U-Boot fix.  Let me
> see if I can put together something that will handle this in RW
> U-Boot.  Even if we don't officially ship this RW U-Boot we can always
> point people at the binaries.
>
>
> Does that make sense?
>
> -Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Doug Anderson]

Abhilash,

On Fri, Jun 6, 2014 at 11:31 AM, Abhilash Kesavan
 wrote:
> Hi Doug,
>
> On Fri, Jun 6, 2014 at 11:50 PM, Doug Anderson  wrote:
>> Abhilash,
>>
>>
>>
>> On Fri, Jun 6, 2014 at 11:12 AM, Abhilash Kesavan
>>  wrote:
>>> Hi Doug,
>>>
>>> On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  wrote:
 Abhilash,

 On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
  wrote:
> Hi Doug,
>
> The first change in the kernel (clearing an iRAM location) is needed
> because of an unnecessary change that we are carrying in the Chrome
> U-boot. There is no reason for us to have the workaround in the
> mainline kernel. Rather, we should remove the check from our u-boot.
> However AFAIR a clean-up patch that I had posted internally was not
> accepted as we had frozen the SPL at the time.

 Ah, is that this one, or a different one?

 https://chromium-review.googlesource.com/#/c/66049/
>>> Yes, this along with a kernel side change.
>>
>> Can we safely take this one without the kernel-side one?
> Yes, just the u-boot change should suffice.
>>
>>
 If we land that patch now it won't help since nobody is going to be
 updating their read-only firmware.  We'll need to put code somewhere
 that fixes it.
>>> We just carry the workaround fix locally until we migrate to mainline
>>> u-boot for 5420 where the unnecessay check will not be present.
>>
>> I think there are people out there who want to run a mainline kernel
>> on existing Chromebook 2 hardware and don't want to rewrite their RO
>> firmware.  We need a solution for those people.
> Yes, I see your point. But, do you think someone who has changed the
> existing fused kernel on the device to a mainline one would be averse
> to applying a couple of small work-around changes as well ? Their
> finding this thread and the proposed "magic" fixes may be difficult
> but not the actual application I think.
>
> How about having a page similar to
> "http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/using-an-upstream-kernel-on-snow";
> for Peach ? We could have the work-arounds listed there.

We can (though the fewer weird things we have the better), but we
definitely need to provide workarounds that don't require people to
change their RO firmware.

Thinking all that through, I think the answer is that we want to
abandon the U-Boot change above
.  At this point
we're never going to take it at this point and there's no possible way
to do it in an RW firmware update (right?) since any workaround would
be overwritten by the SPL at resume time.

So the proper "fix" for the "mw.l 02073028 0" is a kernel fix.  ...and
if upstream doesn't want land it because it's impure then we'll just
have to list it on our "apply these hacks to your kernel".  You sent
me this as a kernel fix before and now I think I understand why (to
handle the s2r case).  Can you please post this up to the mailing
list?  Please make sure it will handle the suspend/resume case
whenever suspend/resume starts working (I haven't tested but I
remember hearing that it doesn't work).

Note: I think there are actually two possible kernel fixes (right?):
1. Have the kernel itself (re)write the code at 0x02073000 at bootup /
resume time.  I don't know of any reason why this should need to be in
U-Boot.  Maybe upstream would take this?

2. Have the kernel clear this flag to work with existing Chromebook 2 firmware.

--

The proper "fix" for the "mw.l 10d25000 3" is a U-Boot fix.  Let me
see if I can put together something that will handle this in RW
U-Boot.  Even if we don't officially ship this RW U-Boot we can always
point people at the binaries.


Does that make sense?

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Abhilash Kesavan]

Hi Doug,

On Fri, Jun 6, 2014 at 11:50 PM, Doug Anderson  wrote:
> Abhilash,
>
>
>
> On Fri, Jun 6, 2014 at 11:12 AM, Abhilash Kesavan
>  wrote:
>> Hi Doug,
>>
>> On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  wrote:
>>> Abhilash,
>>>
>>> On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
>>>  wrote:
 Hi Doug,

 The first change in the kernel (clearing an iRAM location) is needed
 because of an unnecessary change that we are carrying in the Chrome
 U-boot. There is no reason for us to have the workaround in the
 mainline kernel. Rather, we should remove the check from our u-boot.
 However AFAIR a clean-up patch that I had posted internally was not
 accepted as we had frozen the SPL at the time.
>>>
>>> Ah, is that this one, or a different one?
>>>
>>> https://chromium-review.googlesource.com/#/c/66049/
>> Yes, this along with a kernel side change.
>
> Can we safely take this one without the kernel-side one?
Yes, just the u-boot change should suffice.
>
>
>>> If we land that patch now it won't help since nobody is going to be
>>> updating their read-only firmware.  We'll need to put code somewhere
>>> that fixes it.
>> We just carry the workaround fix locally until we migrate to mainline
>> u-boot for 5420 where the unnecessay check will not be present.
>
> I think there are people out there who want to run a mainline kernel
> on existing Chromebook 2 hardware and don't want to rewrite their RO
> firmware.  We need a solution for those people.
Yes, I see your point. But, do you think someone who has changed the
existing fused kernel on the device to a mainline one would be averse
to applying a couple of small work-around changes as well ? Their
finding this thread and the proposed "magic" fixes may be difficult
but not the actual application I think.

How about having a page similar to
"http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/using-an-upstream-kernel-on-snow";
for Peach ? We could have the work-arounds listed there.
>
>
 The second change is to enable snoops for boot cluster. Internally, we
 were disabling the snoops for both the clusters at power off and
 enabling it in power_up_setup and power_up. However, I dropped the
 approach due to problems pointed out by Nicolas here
 http://www.spinics.net/lists/arm-kernel/msg324091.html related to
 cpuidle. Hence, we turn it on at the u-boot.
>>>
>>> I think I followed all that.  What you're saying is that our kernel
>>> dynamically enables and disables snoops as needed, but Nicolas pointed
>>> out that it was unsafe (though apparently we're not seeing problems in
>>> our usage).
>> We did not see any problems as CPUIdle was one of the problematic
>> scenarios which we have not got enabled.
>
> Ah, makes sense!
>
>
>
> I'm still trying to figure out all of this code, but we'll also need
> to make sure whatever solution we come up with handles suspend/resume
> properly.  I know SRAM is lost across suspend/resume so someone
> (either the SPL from read-only memory or the kernel) must be
> recreating the SRAM structures after S2R...
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Doug Anderson]

Abhilash,



On Fri, Jun 6, 2014 at 11:12 AM, Abhilash Kesavan
 wrote:
> Hi Doug,
>
> On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  wrote:
>> Abhilash,
>>
>> On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
>>  wrote:
>>> Hi Doug,
>>>
>>> The first change in the kernel (clearing an iRAM location) is needed
>>> because of an unnecessary change that we are carrying in the Chrome
>>> U-boot. There is no reason for us to have the workaround in the
>>> mainline kernel. Rather, we should remove the check from our u-boot.
>>> However AFAIR a clean-up patch that I had posted internally was not
>>> accepted as we had frozen the SPL at the time.
>>
>> Ah, is that this one, or a different one?
>>
>> https://chromium-review.googlesource.com/#/c/66049/
> Yes, this along with a kernel side change.

Can we safely take this one without the kernel-side one?


>> If we land that patch now it won't help since nobody is going to be
>> updating their read-only firmware.  We'll need to put code somewhere
>> that fixes it.
> We just carry the workaround fix locally until we migrate to mainline
> u-boot for 5420 where the unnecessay check will not be present.

I think there are people out there who want to run a mainline kernel
on existing Chromebook 2 hardware and don't want to rewrite their RO
firmware.  We need a solution for those people.


>>> The second change is to enable snoops for boot cluster. Internally, we
>>> were disabling the snoops for both the clusters at power off and
>>> enabling it in power_up_setup and power_up. However, I dropped the
>>> approach due to problems pointed out by Nicolas here
>>> http://www.spinics.net/lists/arm-kernel/msg324091.html related to
>>> cpuidle. Hence, we turn it on at the u-boot.
>>
>> I think I followed all that.  What you're saying is that our kernel
>> dynamically enables and disables snoops as needed, but Nicolas pointed
>> out that it was unsafe (though apparently we're not seeing problems in
>> our usage).
> We did not see any problems as CPUIdle was one of the problematic
> scenarios which we have not got enabled.

Ah, makes sense!



I'm still trying to figure out all of this code, but we'll also need
to make sure whatever solution we come up with handles suspend/resume
properly.  I know SRAM is lost across suspend/resume so someone
(either the SPL from read-only memory or the kernel) must be
recreating the SRAM structures after S2R...
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Abhilash Kesavan]

Hi Doug,

On Fri, Jun 6, 2014 at 11:32 PM, Doug Anderson  wrote:
> Abhilash,
>
> On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
>  wrote:
>> Hi Doug,
>>
>> The first change in the kernel (clearing an iRAM location) is needed
>> because of an unnecessary change that we are carrying in the Chrome
>> U-boot. There is no reason for us to have the workaround in the
>> mainline kernel. Rather, we should remove the check from our u-boot.
>> However AFAIR a clean-up patch that I had posted internally was not
>> accepted as we had frozen the SPL at the time.
>
> Ah, is that this one, or a different one?
>
> https://chromium-review.googlesource.com/#/c/66049/
Yes, this along with a kernel side change.
>
>
> If we land that patch now it won't help since nobody is going to be
> updating their read-only firmware.  We'll need to put code somewhere
> that fixes it.
We just carry the workaround fix locally until we migrate to mainline
u-boot for 5420 where the unnecessay check will not be present.
>
>
>> The second change is to enable snoops for boot cluster. Internally, we
>> were disabling the snoops for both the clusters at power off and
>> enabling it in power_up_setup and power_up. However, I dropped the
>> approach due to problems pointed out by Nicolas here
>> http://www.spinics.net/lists/arm-kernel/msg324091.html related to
>> cpuidle. Hence, we turn it on at the u-boot.
>
> I think I followed all that.  What you're saying is that our kernel
> dynamically enables and disables snoops as needed, but Nicolas pointed
> out that it was unsafe (though apparently we're not seeing problems in
> our usage).
We did not see any problems as CPUIdle was one of the problematic
scenarios which we have not got enabled.
>
> ...so now the kernel doesn't touch the snoops and assumes that U-Boot
> turned them on.
The kernel does turn on the snoops for the incoming cluster.
>
>
> Ugh.


Regards,
Abhilash
>
> -Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Doug Anderson]

Abhilash,

On Fri, Jun 6, 2014 at 10:36 AM, Abhilash Kesavan
 wrote:
> Hi Doug,
>
> The first change in the kernel (clearing an iRAM location) is needed
> because of an unnecessary change that we are carrying in the Chrome
> U-boot. There is no reason for us to have the workaround in the
> mainline kernel. Rather, we should remove the check from our u-boot.
> However AFAIR a clean-up patch that I had posted internally was not
> accepted as we had frozen the SPL at the time.

Ah, is that this one, or a different one?

https://chromium-review.googlesource.com/#/c/66049/


If we land that patch now it won't help since nobody is going to be
updating their read-only firmware.  We'll need to put code somewhere
that fixes it.


> The second change is to enable snoops for boot cluster. Internally, we
> were disabling the snoops for both the clusters at power off and
> enabling it in power_up_setup and power_up. However, I dropped the
> approach due to problems pointed out by Nicolas here
> http://www.spinics.net/lists/arm-kernel/msg324091.html related to
> cpuidle. Hence, we turn it on at the u-boot.

I think I followed all that.  What you're saying is that our kernel
dynamically enables and disables snoops as needed, but Nicolas pointed
out that it was unsafe (though apparently we're not seeing problems in
our usage).

...so now the kernel doesn't touch the snoops and assumes that U-Boot
turned them on.


Ugh.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Abhilash Kesavan]

Hi Doug,

The first change in the kernel (clearing an iRAM location) is needed
because of an unnecessary change that we are carrying in the Chrome
U-boot. There is no reason for us to have the workaround in the
mainline kernel. Rather, we should remove the check from our u-boot.
However AFAIR a clean-up patch that I had posted internally was not
accepted as we had frozen the SPL at the time.

The second change is to enable snoops for boot cluster. Internally, we
were disabling the snoops for both the clusters at power off and
enabling it in power_up_setup and power_up. However, I dropped the
approach due to problems pointed out by Nicolas here
http://www.spinics.net/lists/arm-kernel/msg324091.html related to
cpuidle. Hence, we turn it on at the u-boot.

Regards,
Abhilash

On Fri, Jun 6, 2014 at 10:47 PM, Doug Anderson  wrote:
> Tushar,
>
> On Thu, Jun 5, 2014 at 9:38 PM, Tushar Behera  wrote:
>> On 06/06/2014 06:38 AM, Doug Anderson wrote:
>>> Hi,
>>>
>>> When I try to boot linuxnext on my exynos5420-peach-pit chromebook I
>>> have problems bringing up extra CPUs:
>>>
>>> 1. They don't come up
>>>
>>
>> [ ... ]
>>
>>> [1.125030] CPU1: failed to boot: -38
>>> [2.130043] CPU2: failed to boot: -38
>>> [3.135059] CPU3: failed to boot: -38
>>>
>>
>> With following config modifications over exynos_defconfig, I can see 4
>> big cores coming up.
>>
>> CONFIG_MCPM=y
>> CONFIG_EXYNOS5420_MCPM=y
>
> Thanks!  OK, that certainly changes the behavior for me!  ...but it
> doesn't work.
>
> I also got a response from some folks that I pinged off-list.
> Apparently our U-Boot doesn't set things up like the upstream kernel
> expects (ugh!), so to get things booting you need the magic commands
> in U-Boot:
>
> mw.l 02073028 0  # Clear EXYNOS_START_FLAG
> mw.l 10d25000 3  # Enable CCI from U-Boot
>
> Once you do that then all 8 CPUs can come up!
>
>
> Can someone at Samsung come up with a reasonable solution about how we
> should solve this in a way that everyone can be happy with?  I haven't
> really been involved with MCPM stuff, so I'm not sure why what landed
> upstream is different than what we have in our kernel.
>
> I guess worst case we can require that anyone running an upstream
> kernel simply needs a "fixed" U-Boot, but that's always a bit of a
> hassle.  We might have to go there eventually anyway given that our L2
> cache timings in U-Boot are wrong (again!) and it's unlikely that
> upstream will take some weird hack in the kernel to fix them...
>
> -Doug
>
>
> P.S. We also really need to do something about the configs.  There are
> no exynos configs upstream that have any useful set of options (that I
> know of).  ...and useful options don't seem to get selected
> automatically...
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Doug Anderson]

Tushar,

On Thu, Jun 5, 2014 at 9:38 PM, Tushar Behera  wrote:
> On 06/06/2014 06:38 AM, Doug Anderson wrote:
>> Hi,
>>
>> When I try to boot linuxnext on my exynos5420-peach-pit chromebook I
>> have problems bringing up extra CPUs:
>>
>> 1. They don't come up
>>
>
> [ ... ]
>
>> [1.125030] CPU1: failed to boot: -38
>> [2.130043] CPU2: failed to boot: -38
>> [3.135059] CPU3: failed to boot: -38
>>
>
> With following config modifications over exynos_defconfig, I can see 4
> big cores coming up.
>
> CONFIG_MCPM=y
> CONFIG_EXYNOS5420_MCPM=y

Thanks!  OK, that certainly changes the behavior for me!  ...but it
doesn't work.

I also got a response from some folks that I pinged off-list.
Apparently our U-Boot doesn't set things up like the upstream kernel
expects (ugh!), so to get things booting you need the magic commands
in U-Boot:

mw.l 02073028 0  # Clear EXYNOS_START_FLAG
mw.l 10d25000 3  # Enable CCI from U-Boot

Once you do that then all 8 CPUs can come up!


Can someone at Samsung come up with a reasonable solution about how we
should solve this in a way that everyone can be happy with?  I haven't
really been involved with MCPM stuff, so I'm not sure why what landed
upstream is different than what we have in our kernel.

I guess worst case we can require that anyone running an upstream
kernel simply needs a "fixed" U-Boot, but that's always a bit of a
hassle.  We might have to go there eventually anyway given that our L2
cache timings in U-Boot are wrong (again!) and it's unlikely that
upstream will take some weird hack in the kernel to fix them...

-Doug


P.S. We also really need to do something about the configs.  There are
no exynos configs upstream that have any useful set of options (that I
know of).  ...and useful options don't seem to get selected
automatically...
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with >1 CPU

[Tushar Behera]

On 06/06/2014 06:38 AM, Doug Anderson wrote:
> Hi,
> 
> When I try to boot linuxnext on my exynos5420-peach-pit chromebook I
> have problems bringing up extra CPUs:
> 
> 1. They don't come up
> 

[ ... ]

> [1.125030] CPU1: failed to boot: -38
> [2.130043] CPU2: failed to boot: -38
> [3.135059] CPU3: failed to boot: -38
> 

With following config modifications over exynos_defconfig, I can see 4
big cores coming up.

CONFIG_MCPM=y
CONFIG_EXYNOS5420_MCPM=y

[0.030919] Exynos MCPM support installed
[0.060233] CPU1: Booted secondary processor
[0.090006] CPU1: update cpu_power 1535
[0.090010] CPU1: thread -1, cpu 1, socket 0, mpidr 8001
[0.100234] CPU2: Booted secondary processor
[0.130007] CPU2: update cpu_power 1535
[0.130011] CPU2: thread -1, cpu 2, socket 0, mpidr 8002
[0.140233] CPU3: Booted secondary processor
[0.170008] CPU3: update cpu_power 1535
[0.170012] CPU3: thread -1, cpu 3, socket 0, mpidr 8003


-- 
Tushar Behera
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html