diff mbox

[6/9] arm: mvebu: move cache and mvebu-mbus initialization later

Message ID 20130521141621.GU31290@titan.lakedaemon.net (mailing list archive)
State New, archived
Headers show

Commit Message

Jason Cooper May 21, 2013, 2:16 p.m. UTC 
On Tue, May 21, 2013 at 12:33:31PM +0200, Thomas Petazzoni wrote:
> Current, the L2 cache and the mvebu-mbus drivers are initialized at
> ->init_early() time. However, at ->init_early() time, ioremap() only
> works if a static I/O mapping has already been put in place. If it's
> not the case, it tries to do a memory allocation with kmalloc() which
> is not possible so early at this stage of the initialization.
> 
> Since we want to get rid of the static I/O mapping, we cannot
> initialize the L2 cache driver and the mvebu-mbus driver so early. So,
> we move their initialization to the ->init_time() level, which is
> slightly later (so ioremap() works properly), but sufficiently early
> to be before the call of the ->smp_prepare_cpus() hook, which creates
> an address decoding window for the BootROM, which requires the
> mvebu-mbus driver to be properly initialized.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/armada-370-xp.c |   24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

This doesn't apply when based on mvebu/cleanup because of:

  49ed97f ARM: Orion: Remove redundant init_dma_coherent_pool_size()

I tried hacking it up to put it in mvebu/soc-internal_regs for a few
rounds of testing in linux-next during review, however, I'd prefer you
rebase this on top of mvebu/cleanup.

My version ended up looking like:

---8<----

Comments

Thomas Petazzoni May 21, 2013, 3:37 p.m. UTC | #1 
Dear Jason Cooper,

On Tue, 21 May 2013 10:16:21 -0400, Jason Cooper wrote:

> This doesn't apply when based on mvebu/cleanup because of:
> 
>   49ed97f ARM: Orion: Remove redundant init_dma_coherent_pool_size()

Right.

> I tried hacking it up to put it in mvebu/soc-internal_regs for a few
> rounds of testing in linux-next during review, however, I'd prefer you
> rebase this on top of mvebu/cleanup.

So you mean my next version of the "Internal registers switch" should
be based on mvebu/cleanup, right? There will be some next version in
any case, because I just found a bug in the latest patch when booting
from a bootloader that has already done the switch to 0xF1.

> My version ended up looking like:
> 
> ---8<----
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
> index b9319c4..75ebf56 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -44,14 +44,11 @@ static void __init armada_370_xp_map_io(void)
>  
>  static void __init armada_370_xp_timer_and_clk_init(void)
>  {
> +	char *mbus_soc_name;
> +
>  	mvebu_clocks_init();
>  	armada_370_xp_timer_init();
>  	coherency_init();
> -}
> -
> -static void __init armada_370_xp_init_early(void)
> -{
> -	char *mbus_soc_name;
>  
>  	/*
>  	 * This initialization will be replaced by a DT-based
> @@ -87,7 +84,6 @@ DT_MACHINE_START(ARMADA_XP_DT, "Marvell Armada 370/XP (Device Tree)")
>  	.smp		= smp_ops(armada_xp_smp_ops),
>  	.init_machine	= armada_370_xp_dt_init,
>  	.map_io		= armada_370_xp_map_io,
> -	.init_early	= armada_370_xp_init_early,
>  	.init_time	= armada_370_xp_timer_and_clk_init,
>  	.restart	= mvebu_restart,
>  	.dt_compat	= armada_370_xp_dt_compat,

From a quick look your version looks correct. Everything ends up into
the ->init_time() hook, which is correct. The only thing I had left in
->init_early() in my version was the DMA coherent thing, which is no
longer needed.

Thanks!

Thomas
Jason Cooper May 21, 2013, 3:43 p.m. UTC | #2 
Thomas,

On Tue, May 21, 2013 at 05:37:07PM +0200, Thomas Petazzoni wrote:
> On Tue, 21 May 2013 10:16:21 -0400, Jason Cooper wrote:
> > This doesn't apply when based on mvebu/cleanup because of:
> > 
> >   49ed97f ARM: Orion: Remove redundant init_dma_coherent_pool_size()
> 
> Right.
> 
> > I tried hacking it up to put it in mvebu/soc-internal_regs for a few
> > rounds of testing in linux-next during review, however, I'd prefer you
> > rebase this on top of mvebu/cleanup.
> 
> So you mean my next version of the "Internal registers switch" should
> be based on mvebu/cleanup, right?

Yes, sorry I wasn't clear, I meant the series.  You can drop 1-3 as
well for the next version, since I've pulled those.

> There will be some next version in any case, because I just found a
> bug in the latest patch when booting from a bootloader that has
> already done the switch to 0xF1.

Ok, I'll hold off putting it up for testing until v2.

thx,

Jason.
Thomas Petazzoni May 21, 2013, 4:10 p.m. UTC | #3 
Dear Jason Cooper,

On Tue, 21 May 2013 11:43:13 -0400, Jason Cooper wrote:

> > > I tried hacking it up to put it in mvebu/soc-internal_regs for a few
> > > rounds of testing in linux-next during review, however, I'd prefer you
> > > rebase this on top of mvebu/cleanup.
> > 
> > So you mean my next version of the "Internal registers switch" should
> > be based on mvebu/cleanup, right?
> 
> Yes, sorry I wasn't clear, I meant the series.  You can drop 1-3 as
> well for the next version, since I've pulled those.

Ok. 1-2 have been merged in mvebu/fixes, and 3 in mvebu/cleanup. So I
suppose mvebu/cleanup is based on mvebu/fixes ? Or should I base on
mvebu/cleanup and assume when it will lend in mainline, the mvebu/fixes
patches will be there (which doesn't make the thing easy for testing
here, because to test, I actually need all the patches in the same
branch). Just trying to figure the workflow :)

> > There will be some next version in any case, because I just found a
> > bug in the latest patch when booting from a bootloader that has
> > already done the switch to 0xF1.
> 
> Ok, I'll hold off putting it up for testing until v2.

Well, the current version should be ok build-wise, and works with all
currently available Marvell bootloaders. The bug can only be seen with
a bootloader version that has not yet been really released by Marvell.

But anyway, v2 will follow shortly. With this v1, I was mainly hoping
for some feedback, to see if the solution was acceptable or not.

Best regards,

Thomas
Jason Cooper May 21, 2013, 4:37 p.m. UTC | #4 
Thomas,

On Tue, May 21, 2013 at 06:10:18PM +0200, Thomas Petazzoni wrote:
> On Tue, 21 May 2013 11:43:13 -0400, Jason Cooper wrote:
> > > > I tried hacking it up to put it in mvebu/soc-internal_regs for a few
> > > > rounds of testing in linux-next during review, however, I'd prefer you
> > > > rebase this on top of mvebu/cleanup.
> > > 
> > > So you mean my next version of the "Internal registers switch" should
> > > be based on mvebu/cleanup, right?
> > 
> > Yes, sorry I wasn't clear, I meant the series.  You can drop 1-3 as
> > well for the next version, since I've pulled those.
> 
> Ok. 1-2 have been merged in mvebu/fixes, and 3 in mvebu/cleanup. So I
> suppose mvebu/cleanup is based on mvebu/fixes ? Or should I base on
> mvebu/cleanup and assume when it will lend in mainline, the mvebu/fixes
> patches will be there (which doesn't make the thing easy for testing
> here, because to test, I actually need all the patches in the same
> branch). Just trying to figure the workflow :)

For your development, I would checkout v3.10-rc2, merge /fixes, then
merge /cleanup, the put 4-9 on top.  Then, when sending, just do 4-9
with a note about the merge dependency on /cleanup, and the boot
dependency on /fixes.

If you'd like, I can hold off on merging it until I can base the series
on an -rc including patches 1 and 2.  As is, I might move #3 into the
same branch as 4-9, but we'll still have the conflict I highlighted
earlier.  And anyone resolving it will probably miss the .init_early
removal.

> > > There will be some next version in any case, because I just found a
> > > bug in the latest patch when booting from a bootloader that has
> > > already done the switch to 0xF1.
> > 
> > Ok, I'll hold off putting it up for testing until v2.
> 
> Well, the current version should be ok build-wise, and works with all
> currently available Marvell bootloaders. The bug can only be seen with
> a bootloader version that has not yet been really released by Marvell.
> 
> But anyway, v2 will follow shortly. With this v1, I was mainly hoping
> for some feedback, to see if the solution was acceptable or not.

That was a great explanation of a Schrödinger's Register ;-)

thx,

Jason.
Thomas Petazzoni May 21, 2013, 4:44 p.m. UTC | #5 
Dear Jason Cooper,

On Tue, 21 May 2013 12:37:25 -0400, Jason Cooper wrote:

> On Tue, May 21, 2013 at 06:10:18PM +0200, Thomas Petazzoni wrote:
> > On Tue, 21 May 2013 11:43:13 -0400, Jason Cooper wrote:
> > > > > I tried hacking it up to put it in mvebu/soc-internal_regs for a few
> > > > > rounds of testing in linux-next during review, however, I'd prefer you
> > > > > rebase this on top of mvebu/cleanup.
> > > > 
> > > > So you mean my next version of the "Internal registers switch" should
> > > > be based on mvebu/cleanup, right?
> > > 
> > > Yes, sorry I wasn't clear, I meant the series.  You can drop 1-3 as
> > > well for the next version, since I've pulled those.
> > 
> > Ok. 1-2 have been merged in mvebu/fixes, and 3 in mvebu/cleanup. So I
> > suppose mvebu/cleanup is based on mvebu/fixes ? Or should I base on
> > mvebu/cleanup and assume when it will lend in mainline, the mvebu/fixes
> > patches will be there (which doesn't make the thing easy for testing
> > here, because to test, I actually need all the patches in the same
> > branch). Just trying to figure the workflow :)
> 
> For your development, I would checkout v3.10-rc2, merge /fixes, then
> merge /cleanup, the put 4-9 on top.  Then, when sending, just do 4-9
> with a note about the merge dependency on /cleanup, and the boot
> dependency on /fixes.

Ok, sounds good.

> If you'd like, I can hold off on merging it until I can base the series
> on an -rc including patches 1 and 2.  As is, I might move #3 into the
> same branch as 4-9, but we'll still have the conflict I highlighted
> earlier.  And anyone resolving it will probably miss the .init_early
> removal.

Don't worry, the solution you've proposed earlier sounds fine to me.

> > Well, the current version should be ok build-wise, and works with all
> > currently available Marvell bootloaders. The bug can only be seen with
> > a bootloader version that has not yet been really released by Marvell.
> > 
> > But anyway, v2 will follow shortly. With this v1, I was mainly hoping
> > for some feedback, to see if the solution was acceptable or not.
> 
> That was a great explanation of a Schrödinger's Register ;-)

That's a nice name for such a complicated register, indeed :)

Thomas
diff mbox

Patch

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index b9319c4..75ebf56 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -44,14 +44,11 @@  static void __init armada_370_xp_map_io(void)
 
 static void __init armada_370_xp_timer_and_clk_init(void)
 {
+	char *mbus_soc_name;
+
 	mvebu_clocks_init();
 	armada_370_xp_timer_init();
 	coherency_init();
-}
-
-static void __init armada_370_xp_init_early(void)
-{
-	char *mbus_soc_name;
 
 	/*
 	 * This initialization will be replaced by a DT-based
@@ -87,7 +84,6 @@  DT_MACHINE_START(ARMADA_XP_DT, "Marvell Armada 370/XP (Device Tree)")
 	.smp		= smp_ops(armada_xp_smp_ops),
 	.init_machine	= armada_370_xp_dt_init,
 	.map_io		= armada_370_xp_map_io,
-	.init_early	= armada_370_xp_init_early,
 	.init_time	= armada_370_xp_timer_and_clk_init,
 	.restart	= mvebu_restart,
 	.dt_compat	= armada_370_xp_dt_compat,