[PATCH 4/5] ARM: vexpress: Initial RS1 memory map support

[dave.martin@linaro.org (Dave Martin)]

On Wed, Nov 16, 2011 at 04:28:17PM +0000, Pawel Moll wrote:
> On Wed, 2011-11-16 at 15:42 +0000, Dave Martin wrote:
> > > +config ARCH_VEXPRESS_LEGACY
> > Brief comment explaining what this is?
> > > +config ARCH_VEXPRESS_RS1
> > Ditto
> 
> Ok.
> 
> > > +	bool
> > > +
> > >  config ARCH_VEXPRESS_CA9X4
> > >  	bool "Versatile Express Cortex-A9x4 tile"
> > >  	select CPU_V7
> > > @@ -8,6 +16,7 @@ config ARCH_VEXPRESS_CA9X4
> > >  	select ARM_ERRATA_720789
> > >  	select ARM_ERRATA_751472
> > >  	select ARM_ERRATA_753970
> > > +	select ARCH_VEXPRESS_LEGACY
> > 
> > We should add a brief note to the help text explaining the existence
> > of the new combined A9x4/A5x2 DT based support, if were planning to
> > keep both variants.  (Probably wise for now, since people may be
> > temporarily stuck old bootloaders etc.)
> > 
> > If there is no help text yet, we should add some, but it should be
> > kept brief.
> 
> No probs. All suggestions and "templates" welcomed :-)

There were some instances in my previous series.  I think they were
mostly one-liners.

> 
> > 
> > [...]
> > 
> > > diff --git a/arch/arm/mach-vexpress/include/mach/debug-macro.S b/arch/arm/mach-vexpress/include/mach/debug-macro.S
> > > index fd9e6c7..adc94ce 100644
> > > --- a/arch/arm/mach-vexpress/include/mach/debug-macro.S
> > > +++ b/arch/arm/mach-vexpress/include/mach/debug-macro.S
> > > @@ -10,12 +10,41 @@
> > >   * published by the Free Software Foundation.
> > >   */
> > >  
> > > -#define DEBUG_LL_UART_OFFSET	0x00009000
> > > +#define VEXPRESS_PHYS_BASE_LEGACY	0x10000000
> > > +#define VEXPRESS_UART_OFFSET_LEGACY	0x00009000
> > > +
> > > +#define VEXPRESS_PHYS_BASE_RS1		0x1c000000
> > > +#define VEXPRESS_UART_OFFSET_RS1	0x00090000
> > > +
> > > +#define VEXPRESS_VIRT_BASE		0xf8000000
> > >  
> > >  		.macro	addruart,rp,rv,tmp
> > > -		mov	\rp, #DEBUG_LL_UART_OFFSET
> > > -		orr	\rv, \rp, #0xf8000000	@ virtual base
> > > -		orr	\rp, \rp, #0x10000000	@ physical base
> > > +
> > > +		@ Check the MMU state
> > > +#if defined(CONFIG_MMU)
> > > +		mrc	p15, 0, \tmp, c1, c0	@ SCTRL
> > > +		tst	\tmp, #1		@ MMU enabled?
> > > +		moveq	\tmp, #VEXPRESS_PHYS_BASE_LEGACY
> > > +		movne	\tmp, #VEXPRESS_VIRT_BASE
> > > +#else
> > > +		mov	\tmp, #VEXPRESS_PHYS_BASE_LEGACY
> > > +#endif
> > > +
> > > +		@ PL011 present in "legacy place"?
> > > +		orr	\tmp, \tmp, #VEXPRESS_UART_OFFSET_LEGACY
> > > +		ldr	\tmp, [\tmp, #0xfe0]	@ PeriphID0
> > > +		teq	\tmp, #0x11		@ PL011
> > > +
> > > +		@ Legacy memory map
> > > +		moveq	\rp, #VEXPRESS_UART_OFFSET_LEGACY
> > > +		orreq	\rv, \rp, #VEXPRESS_VIRT_BASE
> > > +		orreq	\rp, \rp, #VEXPRESS_PHYS_BASE_LEGACY
> > > +
> > > +		@ RS1 memory map
> > > +		movne	\rp, #VEXPRESS_UART_OFFSET_RS1
> > > +		orrne	\rv, \rp, #VEXPRESS_VIRT_BASE
> > > +		orrne	\rp, \rp, #VEXPRESS_PHYS_BASE_RS1
> > 
> > I will assume that this works :)
> 
> It does, tested :-) The probing order "legacy-then-RS1" is carefully
> selected - it doesn't work the other way round.
> 
> > Without grokking the detail, it looks fairly reasonable.
> 
> I was considering something similar to
> "arch/arm/mach-omap2/include/mach/debug-macro.S", but it made the code
> even more obscure.

I would hope that the LL debug stuff can eventually be tidied up so that
we can choose the right UART via the DT.  But that's really a wishlist
thing.  Until (or unless) that happens, I guess code such as the above
should be fine.

> 
> > [...]
> > 
> > > diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
> > 
> > [...]
> > 
> > >  void __init v2m_dt_init_early(void)
> > > @@ -601,6 +643,10 @@ static struct of_dev_auxdata v2m_dt_auxdata_lookup[] __initdata = {
> > >  	OF_DEV_AUXDATA("arm,vexpress-flash", V2M_NOR0, "physmap-flash",
> > >  			&v2m_flash_data),
> > >  	OF_DEV_AUXDATA("arm,primecell", V2M_MMCI, "mb:mmci", &v2m_mmci_data),
> > > +	/* RS1 memory map */
> > > +	OF_DEV_AUXDATA("arm,vexpress-flash", 0x08000000, "physmap-flash",
> > > +			&v2m_flash_data),
> > > +	OF_DEV_AUXDATA("arm,primecell", 0x1c050000, "mb:mmci", &v2m_mmci_data),
> > 
> > Can we have macros instead of the magic numbers here?
> 
> I don't see any point:
> 1. These value are used only and only here, so #defining them would just
> add extra lines of traffic.
> 2. They correspond to numbers (as in: numbers) in DTS.
> 3. They will disappear once I'm done with MMCI and sysreg bindings. So
> probably long before this series is actually merged...

If as you rightly point out, this stuff is only here temporarily, then
I don't have a strong view on it.

If there will be no reason every to have macros for those addresses,
then it's true that adding them only to remove them later will cause
needless churn.

Cheers
---Dave