Patchwork [v6,0/9] Versatile Express DT support

login
register
mail settings
Submitter Pawel Moll
Date Dec. 15, 2011, 2:02 p.m.
Message ID <1323957761-13553-1-git-send-email-pawel.moll@arm.com>
Download mbox
Permalink /patch/131647/
State New
Headers show

Pull-request

git://git.linaro.org/people/pawelmoll/linux.git vexpress-dt

Comments

Pawel Moll - Dec. 15, 2011, 2:02 p.m.
Hi All,

This is the last (at least this year) version of the patches. I've added
Tested-by: Tixy to the patches that hasn't change since v5. I didn't add
Arnd's and Rob's Acked-bys as the code changed significantly since. If
still applicable, I'll add then on the first opportunity.

Arnd, Russell, if you think that the code is ready enough for 3.3, please
pull from this branch based on today's tip (v3.2-rc5+):

8<---------------------------------------------------------------------
The following changes since commit 55b02d2f4445ad625213817a1736bf2884d32547:

  Merge branch 'drm-fixes' of git://people.freedesktop.org/~airlied/linux (2011-12-14 19:45:40 -0800)

are available in the git repository at:

  git://git.linaro.org/people/pawelmoll/linux.git vexpress-dt
8<---------------------------------------------------------------------

I also have a "vexpress-dt-rmk-devel-stable" branch there, based on
today's Russell's devel-stable. The only difference is "handle_irq"
added to DT machine description.

If there are still some issues I'll address them once I'm back from
holiday (today it's the last day before I loose all access to the
Internet for a month).

Changes since v5:

* As suggested by Russell, DT-based local timers take precedence over
  statically defined ones (so the twd_base if overwritten).

* Minor redactorial changes in DT-based SMP initialization (the logic
  stays the same).

* Added last missing device node to the V2M trees: "arm,vexpress-vram".

* Similarly to "arm,vexpress-cf" added "arm,vexpress-psram" to the
  relevant node, just in case we need to detect it in future.

Tested on:
- V2P-CA9 with ATAGs (both with a ATAGs-only and ATAGs+DT kernels).
- V2P-CA9 with DT
- V2P-CA5s with DT
- V2P-CA15 with DT
- V2F-2XV6 Cortex-A7 SMM with DT

Thanks to all involved for your help!
8<---------------------------------------------------------------------

Pawel Moll (9):
  ARM: versatile: Add missing ENDPROC to headsmp.S
  ARM: vexpress: Get rid of MMIO_P2V
  ARM: versatile: Map local timers using Device Tree when possible
  ARM: vexpress: Use FDT data in platform SMP calls
  ARM: vexpress: Add Device Tree support
  ARM: vexpress: Motherboard RS1 memory map support
  ARM: vexpress: Add Device Tree for V2P-CA5s core tile
  ARM: vexpress: Add Device Tree for V2P-CA9 core tile
  ARM: vexpress: Add Device Tree for V2P-CA15 core tile (TC1 variant)

 Documentation/devicetree/bindings/arm/vexpress.txt |  144 ++++++++++
 arch/arm/Kconfig                                   |    2 +-
 arch/arm/boot/dts/vexpress-v2m-rs1.dtsi            |  201 ++++++++++++++
 arch/arm/boot/dts/vexpress-v2m.dtsi                |  200 ++++++++++++++
 arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts        |  155 +++++++++++
 arch/arm/boot/dts/vexpress-v2p-ca5s.dts            |  160 +++++++++++
 arch/arm/boot/dts/vexpress-v2p-ca9.dts             |  190 +++++++++++++
 arch/arm/include/asm/hardware/arm_timer.h          |    5 +
 arch/arm/mach-realview/platsmp.c                   |    3 +-
 arch/arm/mach-vexpress/Kconfig                     |   45 +++-
 arch/arm/mach-vexpress/Makefile.boot               |    6 +
 arch/arm/mach-vexpress/core.h                      |    9 +-
 arch/arm/mach-vexpress/ct-ca9x4.c                  |   48 +---
 arch/arm/mach-vexpress/include/mach/ct-ca9x4.h     |   13 +-
 arch/arm/mach-vexpress/include/mach/debug-macro.S  |   37 +++-
 arch/arm/mach-vexpress/include/mach/irqs.h         |    2 +-
 arch/arm/mach-vexpress/include/mach/motherboard.h  |   58 +++--
 arch/arm/mach-vexpress/include/mach/uncompress.h   |   13 +-
 arch/arm/mach-vexpress/platsmp.c                   |  153 ++++++++++-
 arch/arm/mach-vexpress/v2m.c                       |  282 ++++++++++++++++++--
 arch/arm/mm/Kconfig                                |    2 +-
 arch/arm/plat-versatile/headsmp.S                  |    1 +
 arch/arm/plat-versatile/localtimer.c               |   12 +
 23 files changed, 1621 insertions(+), 120 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/vexpress.txt
 create mode 100644 arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
 create mode 100644 arch/arm/boot/dts/vexpress-v2m.dtsi
 create mode 100644 arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
 create mode 100644 arch/arm/boot/dts/vexpress-v2p-ca5s.dts
 create mode 100644 arch/arm/boot/dts/vexpress-v2p-ca9.dts
Rob Herring - Dec. 15, 2011, 2:53 p.m.
On 12/15/2011 08:02 AM, Pawel Moll wrote:
> Try to map TWD registers basing on a "arm,smp-twd" Device Tree
> node (compatible value as used in Highbank's DT). This overrides
> existing twd_base value.
> 
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  arch/arm/plat-versatile/localtimer.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-versatile/localtimer.c b/arch/arm/plat-versatile/localtimer.c
> index 0fb3961..8f0dc10 100644
> --- a/arch/arm/plat-versatile/localtimer.c
> +++ b/arch/arm/plat-versatile/localtimer.c
> @@ -11,6 +11,8 @@
>  #include <linux/init.h>
>  #include <linux/smp.h>
>  #include <linux/clockchips.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
>  
>  #include <asm/smp_twd.h>
>  #include <asm/localtimer.h>
> @@ -21,6 +23,16 @@
>   */
>  int __cpuinit local_timer_setup(struct clock_event_device *evt)
>  {
> +#if defined(CONFIG_OF)
> +	struct device_node *node = of_find_compatible_node(NULL,
> +			NULL, "arm,smp-twd");
> +
> +	if (node)
> +		twd_base = of_iomap(node, 0);
> +#endif

I think your previous version was more correct. This is going to find
the node and do ioremap N times where N is the number of cores. It does
work though because that is what I did initially too.

Rob
Pawel Moll - Dec. 15, 2011, 3:25 p.m.
On Thu, 2011-12-15 at 14:53 +0000, Rob Herring wrote:
> > @@ -21,6 +23,16 @@
> >   */
> >  int __cpuinit local_timer_setup(struct clock_event_device *evt)
> >  {
> > +#if defined(CONFIG_OF)
> > +	struct device_node *node = of_find_compatible_node(NULL,
> > +			NULL, "arm,smp-twd");
> > +
> > +	if (node)
> > +		twd_base = of_iomap(node, 0);
> > +#endif
> 
> I think your previous version was more correct. This is going to find
> the node and do ioremap N times where N is the number of cores. It does
> work though because that is what I did initially too.

Right, how about that, then:

@@ -21,6 +23,22 @@
  */
 int __cpuinit local_timer_setup(struct clock_event_device *evt)
 {
+#if defined(CONFIG_OF)
+       static int dt_node_probed;
+
+       if (!dt_node_probed) {
+               struct device_node *node = of_find_compatible_node(NULL,
+                               NULL, "arm,smp-twd");
+
+               if (node)
+                       twd_base = of_iomap(node, 0);
+
+               dt_node_probed = 1;
+       }
+#endif
+       if (!twd_base)
+               return -ENXIO;
+
        evt->irq = IRQ_LOCALTIMER;
        twd_timer_setup(evt);
        return 0;


Cheers!

Paweł
Jon Medhurst (Tixy) - Jan. 10, 2012, 11:13 a.m.
On Thu, 2011-12-15 at 14:02 +0000, Pawel Moll wrote:
> This patch adds generic Versatile Express DT machine description,
> Device Tree description for the motherboard and documentation for
> the bindings.
[...]
> diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
[...]
> +DT_MACHINE_START(VEXPRESS_DT, "ARM Versatile Express")
> +	.map_io		= v2m_dt_map_io,
> +	.init_early	= v2m_dt_init_early,
> +	.init_irq	= v2m_dt_init_irq,
> +	.timer		= &v2m_dt_timer,
> +	.init_machine	= v2m_dt_init,
> +	.dt_compat	= v2m_dt_match,
> +MACHINE_END

The machine name here is missing a '-' that is in the original non
device-tree machine description, which is "ARM-Versatile Express".
The hyphen is also in Integrator and Realview machine names, so this
seems to be a convention with ARM boards.

(I found this because Android parses machine name for configuration
purposes.)
Pawel Moll - Jan. 19, 2012, 1:21 p.m.
Hi,

Sorry about loooong delay - I've been on holiday.

On Wed, 2012-01-04 at 16:35 +0000, David Vrabel wrote:
> On 15/12/11 14:02, Pawel Moll wrote:
> > This patch adds support for RS1 memory map based Versatile Express
> > motherboard.
> > 
> [...]
> > --- 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 DEBUG_LL_PHYS_BASE		0x10000000
> > +#define DEBUG_LL_UART_OFFSET		0x00009000
> > +
> > +#define DEBUG_LL_PHYS_BASE_RS1		0x1c000000
> > +#define DEBUG_LL_UART_OFFSET_RS1	0x00090000
> > +
> > +#define DEBUG_LL_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, #DEBUG_LL_PHYS_BASE
> > +		movne	\tmp, #DEBUG_LL_VIRT_BASE
> > +#else
> > +		mov	\tmp, #DEBUG_LL_PHYS_BASE
> > +#endif
> > +
> > +		@ PL011 present in "original" place?
> > +		orr	\tmp, \tmp, #DEBUG_LL_UART_OFFSET
> > +		ldr	\tmp, [\tmp, #0xfe0]	@ PeriphID0
> 
> This doesn't work with CONFIG_EARLY_PRINTK=y on a system with the RS1
> memory map.  

It does for me:

# zcat /proc/config.gz | grep EARLY_PRINTK
CONFIG_EARLY_PRINTK=y
# cat /proc/device-tree/motherboard/arm,v2m-memory-map && echo
rs1     
#

Can you tell me what exactly is going wrong in your case? Does it hang
without any warning? Do you get at least part of the boot log? Can you
send me (privately probably) your kernel config?

>  __create_page_tables has only mapped the single physical
> page at 0x1c090000 and thus the test for the UART in the other memory
> map faults.

I investigated this when writing the code and I vaguely remember it was
fine, partly by accident. I'll dig in again and let you know my
findings.

Thanks for trying this out!

Paweł
Pawel Moll - Jan. 19, 2012, 1:27 p.m.
On Tue, 2012-01-10 at 14:21 +0000, David Vrabel wrote:
> On 15/12/11 14:02, Pawel Moll wrote:
> > This patch adds Device Tree file for the CoreTile Express A15x2
> > (V2P-CA15) with Test Chip 1.
> 
> This doesn't work as-is with the software model as accessing some of the
> peripherals that aren't modeled will cause an exception.  Is it worth
> having a device tree file suitable for the models? Or are the models too
> configurable for this to be workable?

The model as you have it doesn't exactly represent the board for a
number of reasons, mainly because there was no hardware design when the
model was created, so some of the solution was best-guessed by the model
people. Anyway, current A15 model can't be considered a 1-to-1
equivalent of the VE board. The plan is that the models will be shipped
with their own DTSes. I'll work on that in the following months, I can
keep you updated (and use as a beta tester ;-) if you want.

> > As the chip's GIC has 160 interrupt inputs and equivalent SMM
> > (FPGA) has GIC synthesised with 256 interrupts, NR_IRQS is
> > increased.
> > 
> [...]
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
> [...]
> > +	memory@80000000 {
> > +		device_type = "memory";
> > +		reg = <0x80000000 0x40000000>;
> > +	};
> 
> If CONFIG_ARM_ATAG_DTB_COMPAT is enabled the device tree will end up
> with two nodes describing the memory ("memory" and "memory@80000000" in
> this case).

You're right - the skeleton.dtsi contains "memory" mode... Funnily
enough originally I was using that name, but then Rob Herring suggested
changing it to @80000000, which seemed reasonable.

Now I wonder - is the "memory" node special and should not contain
"@address", or the skelton shouldn't contain the empty "memory" node...

Cheers!

Paweł
Rob Herring - Jan. 19, 2012, 1:34 p.m.
On 01/19/2012 07:27 AM, Pawel Moll wrote:
> On Tue, 2012-01-10 at 14:21 +0000, David Vrabel wrote:
>> On 15/12/11 14:02, Pawel Moll wrote:
>>> This patch adds Device Tree file for the CoreTile Express A15x2
>>> (V2P-CA15) with Test Chip 1.
>>
>> This doesn't work as-is with the software model as accessing some of the
>> peripherals that aren't modeled will cause an exception.  Is it worth
>> having a device tree file suitable for the models? Or are the models too
>> configurable for this to be workable?
> 
> The model as you have it doesn't exactly represent the board for a
> number of reasons, mainly because there was no hardware design when the
> model was created, so some of the solution was best-guessed by the model
> people. Anyway, current A15 model can't be considered a 1-to-1
> equivalent of the VE board. The plan is that the models will be shipped
> with their own DTSes. I'll work on that in the following months, I can
> keep you updated (and use as a beta tester ;-) if you want.
> 
>>> As the chip's GIC has 160 interrupt inputs and equivalent SMM
>>> (FPGA) has GIC synthesised with 256 interrupts, NR_IRQS is
>>> increased.
>>>
>> [...]
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
>> [...]
>>> +	memory@80000000 {
>>> +		device_type = "memory";
>>> +		reg = <0x80000000 0x40000000>;
>>> +	};
>>
>> If CONFIG_ARM_ATAG_DTB_COMPAT is enabled the device tree will end up
>> with two nodes describing the memory ("memory" and "memory@80000000" in
>> this case).
> 
> You're right - the skeleton.dtsi contains "memory" mode... Funnily
> enough originally I was using that name, but then Rob Herring suggested
> changing it to @80000000, which seemed reasonable.
> 
> Now I wonder - is the "memory" node special and should not contain
> "@address", or the skelton shouldn't contain the empty "memory" node...
> 

Hummm... I guess you should just use "memory" if you are using
skeleton.dtsi.

Rob
Pawel Moll - Jan. 19, 2012, 1:43 p.m.
On Thu, 2012-01-19 at 13:34 +0000, Rob Herring wrote:
> > You're right - the skeleton.dtsi contains "memory" mode... Funnily
> > enough originally I was using that name, but then Rob Herring suggested
> > changing it to @80000000, which seemed reasonable.
> > 
> > Now I wonder - is the "memory" node special and should not contain
> > "@address", or the skelton shouldn't contain the empty "memory" node...
> > 
> 
> Hummm... I guess you should just use "memory" if you are using
> skeleton.dtsi.

Well, I don't mind _not_ using skeleton, but I had an impression the
general policy was to use it?

Paweł
Rob Herring - Jan. 19, 2012, 2:01 p.m.
On 01/19/2012 07:43 AM, Pawel Moll wrote:
> On Thu, 2012-01-19 at 13:34 +0000, Rob Herring wrote:
>>> You're right - the skeleton.dtsi contains "memory" mode... Funnily
>>> enough originally I was using that name, but then Rob Herring suggested
>>> changing it to @80000000, which seemed reasonable.
>>>
>>> Now I wonder - is the "memory" node special and should not contain
>>> "@address", or the skelton shouldn't contain the empty "memory" node...
>>>
>>
>> Hummm... I guess you should just use "memory" if you are using
>> skeleton.dtsi.
> 
> Well, I don't mind _not_ using skeleton, but I had an impression the
> general policy was to use it?

Either way is fine. I don't really think it buys you much.

Rob
Pawel Moll - Jan. 19, 2012, 2:51 p.m.
On Thu, 2012-01-19 at 14:01 +0000, Rob Herring wrote:
> On 01/19/2012 07:43 AM, Pawel Moll wrote:
> > On Thu, 2012-01-19 at 13:34 +0000, Rob Herring wrote:
> >>> You're right - the skeleton.dtsi contains "memory" mode... Funnily
> >>> enough originally I was using that name, but then Rob Herring suggested
> >>> changing it to @80000000, which seemed reasonable.
> >>>
> >>> Now I wonder - is the "memory" node special and should not contain
> >>> "@address", or the skelton shouldn't contain the empty "memory" node...
> >>>
> >>
> >> Hummm... I guess you should just use "memory" if you are using
> >> skeleton.dtsi.
> > 
> > Well, I don't mind _not_ using skeleton, but I had an impression the
> > general policy was to use it?
> 
> Either way is fine. I don't really think it buys you much.

Ok, /include/ "skeleton.dtsi" is gone then :-)

Cheers!

Paweł
David Vrabel - Jan. 19, 2012, 4:46 p.m.
On 19/01/12 13:21, Pawel Moll wrote:
> Hi,
> 
> Sorry about loooong delay - I've been on holiday.
> 
> On Wed, 2012-01-04 at 16:35 +0000, David Vrabel wrote:
>> On 15/12/11 14:02, Pawel Moll wrote:
>>> This patch adds support for RS1 memory map based Versatile Express
>>> motherboard.
>>>
>> [...]
>>> --- 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 DEBUG_LL_PHYS_BASE		0x10000000
>>> +#define DEBUG_LL_UART_OFFSET		0x00009000
>>> +
>>> +#define DEBUG_LL_PHYS_BASE_RS1		0x1c000000
>>> +#define DEBUG_LL_UART_OFFSET_RS1	0x00090000
>>> +
>>> +#define DEBUG_LL_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, #DEBUG_LL_PHYS_BASE
>>> +		movne	\tmp, #DEBUG_LL_VIRT_BASE
>>> +#else
>>> +		mov	\tmp, #DEBUG_LL_PHYS_BASE
>>> +#endif
>>> +
>>> +		@ PL011 present in "original" place?
>>> +		orr	\tmp, \tmp, #DEBUG_LL_UART_OFFSET
>>> +		ldr	\tmp, [\tmp, #0xfe0]	@ PeriphID0
>>
>> This doesn't work with CONFIG_EARLY_PRINTK=y on a system with the RS1
>> memory map.  
> 
> It does for me:
> 
> # zcat /proc/config.gz | grep EARLY_PRINTK
> CONFIG_EARLY_PRINTK=y
> # cat /proc/device-tree/motherboard/arm,v2m-memory-map && echo
> rs1     
> #

earlyprintk needs to be on the kernel command line to enable it.
Without this option it will work fine.

> Can you tell me what exactly is going wrong in your case? Does it hang
> without any warning? Do you get at least part of the boot log? Can you
> send me (privately probably) your kernel config?

The only output is from the zImage decompressor.

It's a synchronous data fault and DFAR is 0xf8009fe0.

>>  __create_page_tables has only mapped the single physical
>> page at 0x1c090000 and thus the test for the UART in the other memory
>> map faults.
> 
> I investigated this when writing the code and I vaguely remember it was
> fine, partly by accident. I'll dig in again and let you know my
> findings.

It's also a problem when running as a guest under a hypervisor as there
won't be any stage 2 translation table entries for non-existent peripherals.

I think there needs to be someway of finding out from the DTB which UART
to use.

David
David Vrabel - Jan. 19, 2012, 5 p.m.
On 19/01/12 14:51, Pawel Moll wrote:
> On Thu, 2012-01-19 at 14:01 +0000, Rob Herring wrote:
>> On 01/19/2012 07:43 AM, Pawel Moll wrote:
>>> On Thu, 2012-01-19 at 13:34 +0000, Rob Herring wrote:
>>>>> You're right - the skeleton.dtsi contains "memory" mode... Funnily
>>>>> enough originally I was using that name, but then Rob Herring suggested
>>>>> changing it to @80000000, which seemed reasonable.
>>>>>
>>>>> Now I wonder - is the "memory" node special and should not contain
>>>>> "@address", or the skelton shouldn't contain the empty "memory" node...
>>>>>
>>>>
>>>> Hummm... I guess you should just use "memory" if you are using
>>>> skeleton.dtsi.
>>>
>>> Well, I don't mind _not_ using skeleton, but I had an impression the
>>> general policy was to use it?
>>
>> Either way is fine. I don't really think it buys you much.
> 
> Ok, /include/ "skeleton.dtsi" is gone then :-)

The problem wasn't with including skeleton.dtsi.  With
CONFIG_ARM_ATAG_DTB_COMPAT the zImage decompressor modifies the appended
DTB using information from the ATAGs (see atags_to_fdt()).

If there's an ATAG giving the amount of RAM the DTB's "memory" node is
replaced with a new one.  Since the vexpress DTBs don't have a "memory"
node it's added and the DTB ends up with two nodes describing memory.

I don't expect any real production vexpress system to use this config
options -- we're using it now when running as a guest under Xen because
Xen doesn't (yet) support device tree and we're using ATAGs to tell the
guest how much RAM it's been allocated.

David
Russell King - ARM Linux - Jan. 19, 2012, 5:11 p.m.
On Thu, Jan 19, 2012 at 05:00:56PM +0000, David Vrabel wrote:
> I don't expect any real production vexpress system to use this config
> options

I do - _if_ I decide to try DT on my Versatile Express.  That'll be
with the existing boot setup, which will be ATAG based.
Pawel Moll - Jan. 19, 2012, 5:27 p.m.
On Thu, 2012-01-19 at 17:00 +0000, David Vrabel wrote:
> The problem wasn't with including skeleton.dtsi.  

Including as it is creates two device_type="memory" nodes, one with
regs=<0 0>, which is definitely wrong.

> With
> CONFIG_ARM_ATAG_DTB_COMPAT the zImage decompressor modifies the appended
> DTB using information from the ATAGs (see atags_to_fdt()).
> 
> If there's an ATAG giving the amount of RAM the DTB's "memory" node is
> replaced with a new one.  Since the vexpress DTBs don't have a "memory"
> node it's added and the DTB ends up with two nodes describing memory.

The "memory@address" node name is in my opinion perfectly legal - p. 3.4
of the DT spec says "The name component of the node name (see 2.2.1)
shall be memory.". So the decompressor code may be wrong in looking for
adress-less "memory" node...

One way or the other, I'll get this fixed.

Thanks for letting me know!

Paweł
Pawel Moll - Jan. 19, 2012, 5:31 p.m.
On Thu, 2012-01-19 at 16:46 +0000, David Vrabel wrote:
> > It does for me:
> > 
> > # zcat /proc/config.gz | grep EARLY_PRINTK
> > CONFIG_EARLY_PRINTK=y
> > # cat /proc/device-tree/motherboard/arm,v2m-memory-map && echo
> > rs1     
> > #
> 
> earlyprintk needs to be on the kernel command line to enable it.
> Without this option it will work fine.

# cat /proc/cmdline
console=ttyAMA0,38400 earlyprintk rootwait root=/dev/mmcblk0p2 mmci.fmax=12000000 debug

I'll investigate the matter to the bottom, though. Your kernel config
would be helpful, thanks!

> It's also a problem when running as a guest under a hypervisor as there
> won't be any stage 2 translation table entries for non-existent peripherals.
> 
> I think there needs to be someway of finding out from the DTB which UART
> to use.

Yess... The DT+earlyprintk problem was discussed several times already,
without any happy resolution so far...

Paweł
Russell King - ARM Linux - Jan. 19, 2012, 5:50 p.m.
On Thu, Jan 19, 2012 at 05:27:15PM +0000, Pawel Moll wrote:
> On Thu, 2012-01-19 at 17:00 +0000, David Vrabel wrote:
> > The problem wasn't with including skeleton.dtsi.  
> 
> Including as it is creates two device_type="memory" nodes, one with
> regs=<0 0>, which is definitely wrong.
> 
> > With
> > CONFIG_ARM_ATAG_DTB_COMPAT the zImage decompressor modifies the appended
> > DTB using information from the ATAGs (see atags_to_fdt()).
> > 
> > If there's an ATAG giving the amount of RAM the DTB's "memory" node is
> > replaced with a new one.  Since the vexpress DTBs don't have a "memory"
> > node it's added and the DTB ends up with two nodes describing memory.
> 
> The "memory@address" node name is in my opinion perfectly legal - p. 3.4
> of the DT spec says "The name component of the node name (see 2.2.1)
> shall be memory.". So the decompressor code may be wrong in looking for
> adress-less "memory" node...

I don't think you can expect such early code to properly parse a DT tree
with a variability in how memory stuff is declared into that DT tree.

What if you have two memory nodes specified in the DT file, and the
ATAG data contains one?

The more I look at this, the more I'm convinced that Grant's idea that
DT should entirely override ATAGs all the way to the kernel proper was
the wrong solution - at least in the kernel, if we had both available,
we could make a choice there, and have the full DT library to be able
to manipulate the DT blob.

Unfortunately, that's not so, so we're just going to have to accept
that on ARM it should be "memory" if we want the ATAG code to override
it.  Expecting the decompressor to figure out that it needs to delete
DT nodes and all that I think is asking far too much.
Grant Likely - Jan. 19, 2012, 5:59 p.m.
On Thu, Jan 19, 2012 at 10:50 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jan 19, 2012 at 05:27:15PM +0000, Pawel Moll wrote:
>> On Thu, 2012-01-19 at 17:00 +0000, David Vrabel wrote:
>> > The problem wasn't with including skeleton.dtsi.
>>
>> Including as it is creates two device_type="memory" nodes, one with
>> regs=<0 0>, which is definitely wrong.
>>
>> > With
>> > CONFIG_ARM_ATAG_DTB_COMPAT the zImage decompressor modifies the appended
>> > DTB using information from the ATAGs (see atags_to_fdt()).
>> >
>> > If there's an ATAG giving the amount of RAM the DTB's "memory" node is
>> > replaced with a new one.  Since the vexpress DTBs don't have a "memory"
>> > node it's added and the DTB ends up with two nodes describing memory.
>>
>> The "memory@address" node name is in my opinion perfectly legal - p. 3.4
>> of the DT spec says "The name component of the node name (see 2.2.1)
>> shall be memory.". So the decompressor code may be wrong in looking for
>> adress-less "memory" node...
>
> I don't think you can expect such early code to properly parse a DT tree
> with a variability in how memory stuff is declared into that DT tree.
>
> What if you have two memory nodes specified in the DT file, and the
> ATAG data contains one?

Yes, just because it is technically legal doesn't make it okay.  The
pragmatic approach here is that the skeleton.dtsi file calls the node
"memory", so this .dts file must do the same.

> The more I look at this, the more I'm convinced that Grant's idea that
> DT should entirely override ATAGs all the way to the kernel proper was
> the wrong solution - at least in the kernel, if we had both available,
> we could make a choice there, and have the full DT library to be able
> to manipulate the DT blob.

Hey!  I was originally lobbying for the dt pointer carried by an ATAG.
 Nico conviced me otherwise.  :-)

> Unfortunately, that's not so, so we're just going to have to accept
> that on ARM it should be "memory" if we want the ATAG code to override
> it.  Expecting the decompressor to figure out that it needs to delete
> DT nodes and all that I think is asking far too much.

+1
Nicolas Pitre - Jan. 19, 2012, 6:09 p.m.
On Thu, 19 Jan 2012, Grant Likely wrote:

> On Thu, Jan 19, 2012 at 10:50 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Thu, Jan 19, 2012 at 05:27:15PM +0000, Pawel Moll wrote:
> >> On Thu, 2012-01-19 at 17:00 +0000, David Vrabel wrote:
> >> > The problem wasn't with including skeleton.dtsi.
> >>
> >> Including as it is creates two device_type="memory" nodes, one with
> >> regs=<0 0>, which is definitely wrong.
> >>
> >> > With
> >> > CONFIG_ARM_ATAG_DTB_COMPAT the zImage decompressor modifies the appended
> >> > DTB using information from the ATAGs (see atags_to_fdt()).
> >> >
> >> > If there's an ATAG giving the amount of RAM the DTB's "memory" node is
> >> > replaced with a new one.  Since the vexpress DTBs don't have a "memory"
> >> > node it's added and the DTB ends up with two nodes describing memory.
> >>
> >> The "memory@address" node name is in my opinion perfectly legal - p. 3.4
> >> of the DT spec says "The name component of the node name (see 2.2.1)
> >> shall be memory.". So the decompressor code may be wrong in looking for
> >> adress-less "memory" node...
> >
> > I don't think you can expect such early code to properly parse a DT tree
> > with a variability in how memory stuff is declared into that DT tree.
> >
> > What if you have two memory nodes specified in the DT file, and the
> > ATAG data contains one?
> 
> Yes, just because it is technically legal doesn't make it okay.  The
> pragmatic approach here is that the skeleton.dtsi file calls the node
> "memory", so this .dts file must do the same.
> 
> > The more I look at this, the more I'm convinced that Grant's idea that
> > DT should entirely override ATAGs all the way to the kernel proper was
> > the wrong solution - at least in the kernel, if we had both available,
> > we could make a choice there, and have the full DT library to be able
> > to manipulate the DT blob.
> 
> Hey!  I was originally lobbying for the dt pointer carried by an ATAG.
>  Nico conviced me otherwise.  :-)

Hey!  I was originally lobbying for people to have a fully DT aware 
bootloader if they wanted to play with DT, otherwise there is no 
incentive for updated bootloaders.  But someone else convinced me 
otherwise.  :-)

Mixed bags always have loose ends.


Nicolas
Grant Likely - Jan. 19, 2012, 10:07 p.m.
On Thu, Jan 19, 2012 at 11:09 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Thu, 19 Jan 2012, Grant Likely wrote:
>
>> On Thu, Jan 19, 2012 at 10:50 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Thu, Jan 19, 2012 at 05:27:15PM +0000, Pawel Moll wrote:
>> >> On Thu, 2012-01-19 at 17:00 +0000, David Vrabel wrote:
>> >> > The problem wasn't with including skeleton.dtsi.
>> >>
>> >> Including as it is creates two device_type="memory" nodes, one with
>> >> regs=<0 0>, which is definitely wrong.
>> >>
>> >> > With
>> >> > CONFIG_ARM_ATAG_DTB_COMPAT the zImage decompressor modifies the appended
>> >> > DTB using information from the ATAGs (see atags_to_fdt()).
>> >> >
>> >> > If there's an ATAG giving the amount of RAM the DTB's "memory" node is
>> >> > replaced with a new one.  Since the vexpress DTBs don't have a "memory"
>> >> > node it's added and the DTB ends up with two nodes describing memory.
>> >>
>> >> The "memory@address" node name is in my opinion perfectly legal - p. 3.4
>> >> of the DT spec says "The name component of the node name (see 2.2.1)
>> >> shall be memory.". So the decompressor code may be wrong in looking for
>> >> adress-less "memory" node...
>> >
>> > I don't think you can expect such early code to properly parse a DT tree
>> > with a variability in how memory stuff is declared into that DT tree.
>> >
>> > What if you have two memory nodes specified in the DT file, and the
>> > ATAG data contains one?
>>
>> Yes, just because it is technically legal doesn't make it okay.  The
>> pragmatic approach here is that the skeleton.dtsi file calls the node
>> "memory", so this .dts file must do the same.
>>
>> > The more I look at this, the more I'm convinced that Grant's idea that
>> > DT should entirely override ATAGs all the way to the kernel proper was
>> > the wrong solution - at least in the kernel, if we had both available,
>> > we could make a choice there, and have the full DT library to be able
>> > to manipulate the DT blob.
>>
>> Hey!  I was originally lobbying for the dt pointer carried by an ATAG.
>>  Nico conviced me otherwise.  :-)
>
> Hey!  I was originally lobbying for people to have a fully DT aware
> bootloader if they wanted to play with DT, otherwise there is no
> incentive for updated bootloaders.  But someone else convinced me
> otherwise.  :-)
>
> Mixed bags always have loose ends.

Hahaha.  Having said that though, I still strongly agree with what we
have.  Giving the kernel only one or the other avoids any weirdness
about which the kernel should choose when both are present.  The
bootwrapper atag-to-dt conversion is a migration and development tool,
and it should not do any DT manipulation must be kept to a bare
minimum.

g.
Pawel Moll - Jan. 27, 2012, 2:02 p.m.
On Thu, 2012-01-19 at 16:46 +0000, David Vrabel wrote:
> >>  __create_page_tables has only mapped the single physical
> >> page at 0x1c090000 and thus the test for the UART in the other memory
> >> map faults.

I got to the bottom of the problem... The mapping created by the
DEBUG_LL code in __create_page_tables is actually a section (1MB) so
covers both possible accesses (0xf8009000 and 0xf8090000). The
difference between model and hardware is that "real" RS1 VE has a DAP
ROM located between 0xf8000000 and 0xf800ffff so the 0xf8009000 succeeds
(returning some irrelevant data), while model is "empty" in that space,
so it faults.

Anyway, I have an idea how to solve (or rather work around) the problem
and will get if working on models in the next version of the series.
I'll send you a modified version of the relevant patch before that so
you can test it.

Thanks for your time!

Paweł
Dave Martin - Jan. 30, 2012, 5:26 p.m.
On Thu, Jan 19, 2012 at 01:21:06PM +0000, Pawel Moll wrote:
> Hi,
> 
> Sorry about loooong delay - I've been on holiday.
> 
> On Wed, 2012-01-04 at 16:35 +0000, David Vrabel wrote:
> > On 15/12/11 14:02, Pawel Moll wrote:
> > > This patch adds support for RS1 memory map based Versatile Express
> > > motherboard.
> > > 
> > [...]
> > > --- 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 DEBUG_LL_PHYS_BASE		0x10000000
> > > +#define DEBUG_LL_UART_OFFSET		0x00009000
> > > +
> > > +#define DEBUG_LL_PHYS_BASE_RS1		0x1c000000
> > > +#define DEBUG_LL_UART_OFFSET_RS1	0x00090000
> > > +
> > > +#define DEBUG_LL_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, #DEBUG_LL_PHYS_BASE
> > > +		movne	\tmp, #DEBUG_LL_VIRT_BASE
> > > +#else
> > > +		mov	\tmp, #DEBUG_LL_PHYS_BASE
> > > +#endif
> > > +
> > > +		@ PL011 present in "original" place?
> > > +		orr	\tmp, \tmp, #DEBUG_LL_UART_OFFSET
> > > +		ldr	\tmp, [\tmp, #0xfe0]	@ PeriphID0
> > 
> > This doesn't work with CONFIG_EARLY_PRINTK=y on a system with the RS1
> > memory map.  
> 
> It does for me:
> 
> # zcat /proc/config.gz | grep EARLY_PRINTK
> CONFIG_EARLY_PRINTK=y
> # cat /proc/device-tree/motherboard/arm,v2m-memory-map && echo
> rs1     
> #
> 
> Can you tell me what exactly is going wrong in your case? Does it hang
> without any warning? Do you get at least part of the boot log? Can you
> send me (privately probably) your kernel config?

I had to disable this when running on the ARM fast model with your vexpress
DT series.  I didn't debug into exactly why I had to disable it, but I
think I was getting aborts when the kernel was probing for nonexistent
legacy memory map location of the UART.  I don't know whether this will
affect real hardware though... and maybe I was hitting the problem due
to something else.

Anyway unless I've misunderstood something, AMBA devices are only really
probable if there is some real bus slave at the probed address, which is
free from nasty side-effects.

Really we should have a way for getting this info from the device tree,
yada yada.  (Repeating myself here, I know)

In the meantime, I don't see a practical alternative to adding distinct
lowlevel debug UART selection options for the two memory maps ...?


Cheers
---Dave
Dave Martin - Jan. 30, 2012, 5:32 p.m.
On Fri, Jan 27, 2012 at 02:02:40PM +0000, Pawel Moll wrote:
> On Thu, 2012-01-19 at 16:46 +0000, David Vrabel wrote:
> > >>  __create_page_tables has only mapped the single physical
> > >> page at 0x1c090000 and thus the test for the UART in the other memory
> > >> map faults.
> 
> I got to the bottom of the problem... The mapping created by the
> DEBUG_LL code in __create_page_tables is actually a section (1MB) so
> covers both possible accesses (0xf8009000 and 0xf8090000). The
> difference between model and hardware is that "real" RS1 VE has a DAP
> ROM located between 0xf8000000 and 0xf800ffff so the 0xf8009000 succeeds
> (returning some irrelevant data), while model is "empty" in that space,
> so it faults.
> 
> Anyway, I have an idea how to solve (or rather work around) the problem
> and will get if working on models in the next version of the series.
> I'll send you a modified version of the relevant patch before that so
> you can test it.
> 
> Thanks for your time!

I would be interested in that too.

However, I would still prefer the explicit debug UART selection route,
since we already have to do that for many boards and I'm not sure how
much I trust the memory maps of future tiles and models to "evolve"
relative to what currently exists.  Are you sure it won't change again?

If we rely on lucky arrangement of slaves on the bus in order to have
a workaround, sooner or later our luck is liable to run out.

Cheers
---Dave