Date:	Wed, 3 Dec 2014 08:44:55 -0800
From:	Doug Anderson <dianders@...omium.org>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Arnd Bergmann <arnd@...db.de>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Kevin Hilman <khilman@...aro.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Russ Dill <russ.dill@...il.com>,
	Olof Johansson <olof@...om.net>,
	Tomasz Figa <tomasz.figa@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
	Chris Zhong <zyw@...k-chips.com>,
	Sonny Rao <sonnyrao@...omium.org>
Subject: Re: [PATCH] ARM: rockchip: Convert resume code to C

Linus,

On Wed, Dec 3, 2014 at 5:21 AM, Linus Walleij <linus.walleij@...aro.org> wrote:
> On Tue, Dec 2, 2014 at 6:36 PM, Doug Anderson <dianders@...omium.org> wrote:
>
>> Ah, but the TCM stuff has a critical difference from my problem.  By
>> the very definition of TCM you don't need to do relocation.
>>
>> TCM has the magical property that you can assign the physical address.
>> That means that you instantly sidestep multiplatform problems of
>> having SRAM at different physical addresses.
>
> An SRAM behaves like an ITCM and no DTCM. So just
> patch the code to believe it is an ITCM and test it out.
>
> Then I guess you're implictly referring to the fact that the code
> that you're running in your TCM/SRAM will turn of the MMU,
> so it can't be used for remapping.

Right.  In my case the resume code is running at resume time after the
CPUs have lost state and are just coming back on.  All addresses need
to be physical.  Sorry, I should have been explicit.


> This is correct, BUT if you're
> just compiling for one single machine anyway it doesn't matter,
> just use 0xBFEA0000 or wherever it is physically as the phys+virt
> address.
>
> They are just defined like so in arch/arm/include/asm/memory.h:
>
> /*
>  * We fix the TCM memories max 32 KiB ITCM resp DTCM at these
>  * locations
>  */
> #ifdef CONFIG_HAVE_TCM
> #define ITCM_OFFSET     UL(0xfffe0000)
> #define DTCM_OFFSET     UL(0xfffe8000)
> #endif
>
> Patch it and see if it works.

I'm nearly certainly it could be made to work.  ...but the solution I
have also works just fine.  I was looking for something that upstream
would take, and I don't think the solution you're proposing is
something that I could actually land upstream (could it?)

Also: it is possible (probable?) we'll want to have more than one
blob.  One (resume code) may need to fit in the 4K PMU SRAM that's
saved across suspend/resume.  The other (ddrfreq) may be able to go in
the larger SRAM.  I could try to fool the TCM code to handle this,
but...


>>  You can compile the code
>> to assume it's at 0xfffe0000 and it will work on every single machine
>> out there that needs TCM.
>
> Doesn't matter as long as we're not multiplatform! It's more like
> ITCM_OFFSET would be a Kconfig constant and you could change
> it per-platform that need this.
>
>>  So if you've got a generic "udelay"
>> function you could just mark it as a "tcmfunc" and it will work
>> everywhere.  No relocation needed and the compiler knows exactly where
>> things will be.
>
> This would still work as long as you're not multiplatform.

Yup, but I don't think I can land something upstream that's not multiplatform.


> But udelay is probably a bad idea. Maybe sched_clock() or others.

Some type of delay function is incredibly likely to be needed in SRAM
and it's possible to implement this fairly generically by using
arch_counter_get_cntpct() (or you could get the virtual one).


>> 1. It sure seems unlikely that the current TCM solution would scale to
>> multiplatform.  Oh right, Linus W said this in his reply, too.
>
> Yes. But it is no less elegant than this patch AFAICT.

I guess it's a matter of opinion.  What I have could scale to
multiplatform just fine, which I think is a nice benefit.  It also
doesn't involve coopting the TCM code into behaving in a way it wasn't
quite designed to work.  It also is very self-contained.  ...but I can
certainly see your side of the argument, too.


>> you've got SoC_A, SoC_B, and SoC_C all marking their functions
>> "tcmfunc" then they'll all be placed in the TCM section, right?
>> There's no way to detect that you're on SoC_A and that you only need
>> the SoC_A code.  Given the marching orders of multiplatform,
>> multiplatform, multiplatform then I _think_ that means we shouldn't
>> let anyone merge any code to mainline that uses TCM (unless TCM gets
>> revamped).
>>
>> 2. I haven't tried it, but it seems like the compiler still might not
>> catch stray (accidental) accesses from the TCM section to the non-TCM
>> section.  Again, this isn't a showstopper because you'd just track
>> each one down, but it is a nice feature of adding a separate
>> executable.
>
> This is correct I think.
>
> I think something in the direction of Russ Dill's patch set is the right
> way forward to fix the problem for multiplatform.

Sure.  I look forward to someone solving it.  ...and as I said, until
someone does solve it I think we're blocked from landing deep sleep
mode for rk3288 upstream (unless someone decides that my patches are
OK).

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists