Patchwork ARM: vexpress: initial device tree support

login
register
mail settings
Submitter Dave Martin
Date Sept. 21, 2011, 9:19 a.m.
Message ID <1316596786-2539-1-git-send-email-dave.martin@linaro.org>
Download mbox | patch
Permalink /patch/115719/
State New
Headers show

Comments

Dave Martin - Sept. 21, 2011, 9:19 a.m.
This patch implements initial support for booting using a flattened
device tree on the Versatile Express platform.

Eventually, it should be possible to present a single, core-tile-
independent board, but in this transitional patch the baseboard +
Cortex-A9x4 core tile combination is the only directly supported
platform, since the implementation is not yet fully generic.

For now, clocks and timers are not handled via the device tree.
Implementation of these can follow in later patches.

Thanks to Lorenzo Pieralisi, Grant Likely and Paweł Moll for their
help and contributions.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
Acked-by: Paweł Moll <Pawel.Moll@arm.com>
---

There are some outstanding issues which need to be discussed, listed
below.

  * This patch is not currently based on the GIC bindings being
    discussed by Rob Herring et al.  Once that discussion reaches a
    conclusion, it should be straightforward to rebase onto the result.

  * The following added bindings are not present upstream and need
    documentation / discussion:

      * arm,vexpress -- the global board binding for all platform
        combinations using the Versatile Express motherboard.

      * arm,vexpress-v2p-ca9 -- the specific binding for the Versatile
        Express motherboard with Cortex-A9x4 core tile installed.  It
        is only mentioned as the most-specific match in vexpress-v2p-
        ca9.dts

        Since it's intended that the motherboard code should be fully
        generic, and because no other core tiles are upstream yet,
        perhaps we can get rid of this binding right away.

      * edid -- It should be possible to have a fairly generic binding
        for EDID interfaces, but none seems to exist yet.  Discussion
        is needed regarding what form this should take.

        This might more appropriately be called "ddc" (or some
        variation on that), since EDID seems only to describe the
        format of the ID data retrievable via this interface; not the
        interface itself.

      * arm,vexpress-flash -- Needed because of the requirement to
        provide the physmap_flash driver with a special .set_vpp
        handler.

      * idt,89hpes32h8 -- This is the IDT 89HPES32H8 PCI express
        interconnect switch.  This isn't needed for the Versatile
        Express to work, but would be needed if using PCI-e peripherals
        for real.  I expect that more driver support needs to go
        upstream before this is actually usable.

      * nxp,isp1761 -- The driver support for this is already upstream
        (with some minor issues for ARM support).

      * arm,amba-bus -- widely used by other boards and patchsets, but
        seems not to be documented.

      * The following bindings for ARM primecell peripherals are used
        elsewhere but not documented.  They should be pretty simple and
        uncontraversial.
          * arm,pl031
          * arm,pl041
          * arm,pl050
          * arm,pl180
          * arm,sp805

        Rob Herring suggested documenting simple bindings for these
        (and others) along with his initial amba device tree probe
        patches, but these bindings don't seem to be documented
        upstream for now.


  * Shawn Guo's smsc911x patch is needed for Ethernet to work.  This is
    headed upstream but not yet in mainline.  It is available in -next.

  * Minor patches are needed to the isp1760 and pata_generic drivers,
    to allow OF-based initialisation across a wider group of
    architectures.  These are being discussed independently, but are
    not yet accepted for merging upstream.

  * Most core-tile peripherals are currently not described in the core-
    tile device tree fragment.  This is a lower-priority issue since
    the motherboard code already autodetects the core-tile (though only
    one core-tile is fully upstream at the moment).

  * Static peripheral mappings are not yet handled in a generic way in
    the board support code.  This is a prerequisite for supporting
    multiple core-tiles int the same kernel.  It well need to get fixed
    later, when extra core tile support is merged (or before).

    Paweł Moll is looking into this separately.

  * The Kconfig logic for ensuring that at least one boot protocol and
    at least one core tile are selected is a bit ugly.  Suggestions for
    improving this are certainly welcome.

 arch/arm/Kconfig                           |    1 +
 arch/arm/boot/dts/vexpress-v2m-legacy.dtsi |  163 ++++++++++++++++++++++++++++
 arch/arm/boot/dts/vexpress-v2p-ca9.dts     |   80 ++++++++++++++
 arch/arm/configs/vexpress_defconfig        |    1 +
 arch/arm/mach-vexpress/Kconfig             |   45 ++++++++-
 arch/arm/mach-vexpress/ct-ca9x4.c          |    7 ++
 arch/arm/mach-vexpress/v2m.c               |   54 +++++++++-
 7 files changed, 349 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/boot/dts/vexpress-v2m-legacy.dtsi
 create mode 100644 arch/arm/boot/dts/vexpress-v2p-ca9.dts
Rob Herring - Sept. 21, 2011, 1:24 p.m.
Dave,

On 09/21/2011 04:19 AM, Dave Martin wrote:
> This patch implements initial support for booting using a flattened
> device tree on the Versatile Express platform.
> 
> Eventually, it should be possible to present a single, core-tile-
> independent board, but in this transitional patch the baseboard +
> Cortex-A9x4 core tile combination is the only directly supported
> platform, since the implementation is not yet fully generic.
> 
> For now, clocks and timers are not handled via the device tree.
> Implementation of these can follow in later patches.
> 
> Thanks to Lorenzo Pieralisi, Grant Likely and Paweł Moll for their
> help and contributions.
> 
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> Acked-by: Paweł Moll <Pawel.Moll@arm.com>
> ---
> 
> There are some outstanding issues which need to be discussed, listed
> below.
> 
>   * This patch is not currently based on the GIC bindings being
>     discussed by Rob Herring et al.  Once that discussion reaches a
>     conclusion, it should be straightforward to rebase onto the result.
> 
>   * The following added bindings are not present upstream and need
>     documentation / discussion:
> 
>       * arm,vexpress -- the global board binding for all platform
>         combinations using the Versatile Express motherboard.
> 
>       * arm,vexpress-v2p-ca9 -- the specific binding for the Versatile
>         Express motherboard with Cortex-A9x4 core tile installed.  It
>         is only mentioned as the most-specific match in vexpress-v2p-
>         ca9.dts
> 
>         Since it's intended that the motherboard code should be fully
>         generic, and because no other core tiles are upstream yet,
>         perhaps we can get rid of this binding right away.
> 
>       * edid -- It should be possible to have a fairly generic binding
>         for EDID interfaces, but none seems to exist yet.  Discussion
>         is needed regarding what form this should take.
> 
>         This might more appropriately be called "ddc" (or some
>         variation on that), since EDID seems only to describe the
>         format of the ID data retrievable via this interface; not the
>         interface itself.
> 
>       * arm,vexpress-flash -- Needed because of the requirement to
>         provide the physmap_flash driver with a special .set_vpp
>         handler.
> 
>       * idt,89hpes32h8 -- This is the IDT 89HPES32H8 PCI express
>         interconnect switch.  This isn't needed for the Versatile
>         Express to work, but would be needed if using PCI-e peripherals
>         for real.  I expect that more driver support needs to go
>         upstream before this is actually usable.
> 
>       * nxp,isp1761 -- The driver support for this is already upstream
>         (with some minor issues for ARM support).
> 
>       * arm,amba-bus -- widely used by other boards and patchsets, but
>         seems not to be documented.
> 

This should be dropped. There's not really any bus component to an amba
bus. All the probing info is within the primecell peripherals.

>       * The following bindings for ARM primecell peripherals are used
>         elsewhere but not documented.  They should be pretty simple and
>         uncontraversial.
>           * arm,pl031
>           * arm,pl041
>           * arm,pl050
>           * arm,pl180
>           * arm,sp805
> 

Plus pl011, pl010, sp804, pl022, pl061

>         Rob Herring suggested documenting simple bindings for these
>         (and others) along with his initial amba device tree probe
>         patches, but these bindings don't seem to be documented
>         upstream for now.
> 

pl330 went the other route with a file for itself. That may be better to
avoid conflicts. But yes, ARM should document all their peripherals. ;)

I'll do the ones on highbank if you want to do the rest on VExp.

> 
>   * Shawn Guo's smsc911x patch is needed for Ethernet to work.  This is
>     headed upstream but not yet in mainline.  It is available in -next.
> 
>   * Minor patches are needed to the isp1760 and pata_generic drivers,
>     to allow OF-based initialisation across a wider group of
>     architectures.  These are being discussed independently, but are
>     not yet accepted for merging upstream.
> 
>   * Most core-tile peripherals are currently not described in the core-
>     tile device tree fragment.  This is a lower-priority issue since
>     the motherboard code already autodetects the core-tile (though only
>     one core-tile is fully upstream at the moment).
> 
>   * Static peripheral mappings are not yet handled in a generic way in
>     the board support code.  This is a prerequisite for supporting
>     multiple core-tiles int the same kernel.  It well need to get fixed
>     later, when extra core tile support is merged (or before).
> 
>     Paweł Moll is looking into this separately.
> 
>   * The Kconfig logic for ensuring that at least one boot protocol and
>     at least one core tile are selected is a bit ugly.  Suggestions for
>     improving this are certainly welcome.
> 
>  arch/arm/Kconfig                           |    1 +
>  arch/arm/boot/dts/vexpress-v2m-legacy.dtsi |  163 ++++++++++++++++++++++++++++
>  arch/arm/boot/dts/vexpress-v2p-ca9.dts     |   80 ++++++++++++++
>  arch/arm/configs/vexpress_defconfig        |    1 +
>  arch/arm/mach-vexpress/Kconfig             |   45 ++++++++-
>  arch/arm/mach-vexpress/ct-ca9x4.c          |    7 ++
>  arch/arm/mach-vexpress/v2m.c               |   54 +++++++++-
>  7 files changed, 349 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/boot/dts/vexpress-v2m-legacy.dtsi
>  create mode 100644 arch/arm/boot/dts/vexpress-v2p-ca9.dts
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 5ebc5d9..a6e90d5 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -282,6 +282,7 @@ config ARCH_VERSATILE
>  
>  config ARCH_VEXPRESS
>  	bool "ARM Ltd. Versatile Express family"
> +	select ARCH_VEXPRESS_SANE_CONFIG
>  	select ARCH_WANT_OPTIONAL_GPIOLIB
>  	select ARM_AMBA
>  	select ARM_TIMER_SP804
> diff --git a/arch/arm/boot/dts/vexpress-v2m-legacy.dtsi b/arch/arm/boot/dts/vexpress-v2m-legacy.dtsi
> new file mode 100644
> index 0000000..fd6e4e4
> --- /dev/null
> +++ b/arch/arm/boot/dts/vexpress-v2m-legacy.dtsi
> @@ -0,0 +1,163 @@
> +// ARM Ltd. Versatile Express Motherboard V2M-P1 (HBI-0190D)
> +// Legacy memory map

Not sure, but C++ style comments are probably frowned upon in dts too.

> +
> +/ {
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;
> +		serial3 = &uart3;
> +		i2c0 = &i2c0;
> +		i2c1 = &i2c1;
> +	};
> +
> +	motherboard {
> +		compatible = "simple-bus";
> +		#address-cells = <2>; // SMB chipselect number and offset
> +		#size-cells = <1>;
> +		#interrupt-cells = <1>;
> +
> +		flash@0,00000000 {
> +			compatible = "arm,vexpress-flash", "cfi-flash";
> +			reg = <0 0x00000000 0x04000000
> +			       1 0x00000000 0x04000000>;
> +			bank-width = <4>;
> +		};
> +
> +		psram@2,00000000 {
> +			compatible = "mtd-ram";
> +			reg = <2 0x00000000 0x02000000>;
> +			bank-width = <4>;
> +		};
> +
> +		ethernet@3,02000000 {
> +			compatible = "smsc,lan9118", "smsc,lan9115";
> +			reg = <3 0x02000000 0x10000>;
> +			reg-io-width = <4>;
> +			interrupts = <15>;
> +			smsc,irq-active-high;
> +			smsc,irq-push-pull;
> +		};
> +
> +		usb@3,03000000 {
> +			compatible = "nxp,usb-isp1761";
> +			reg = <3 0x03000000 0x20000>;
> +			interrupts = <16>;
> +			port1-otg;
> +		};
> +
> +		peripherals@7,00000000 {
> +			compatible = "arm,amba-bus", "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges = <0 7 0 0x20000>;
> +
> +			// PCI-E I2C bus
> +			i2c0: i2c@02000 {
> +				compatible = "arm,versatile-i2c";
> +				reg = <0x02000 0x1000>;
> +
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				pcie-switch@60 {
> +					compatible = "idt,89hpes32h8";
> +					reg = <0x60>;
> +				};
> +			};
> +
> +			aaci@04000 {
> +				compatible = "arm,pl041", "arm,primecell";
> +				reg = <0x04000 0x1000>;
> +				interrupts = <11>;
> +			};
> +
> +			mmci@05000 {
> +				compatible = "arm,pl180", "arm,primecell";
> +				reg = <0x05000 0x1000>;
> +				interrupts = <9 10>;
> +			};
> +
> +			kmi@06000 {
> +				compatible = "arm,pl050", "arm,primecell";
> +				reg = <0x06000 0x1000>;
> +				interrupts = <12>;
> +			};
> +
> +			kmi@07000 {
> +				compatible = "arm,pl050", "arm,primecell";
> +				reg = <0x07000 0x1000>;
> +				interrupts = <13>;
> +			};
> +
> +			uart0: uart@09000 {
> +				compatible = "arm,pl011", "arm,primecell";
> +				reg = <0x09000 0x1000>;
> +				interrupts = <5>;
> +			};
> +
> +			uart1: uart@0a000 {
> +				compatible = "arm,pl011", "arm,primecell";
> +				reg = <0x0a000 0x1000>;
> +				interrupts = <6>;
> +			};
> +
> +			uart2: uart@0b000 {
> +				compatible = "arm,pl011", "arm,primecell";
> +				reg = <0x0b000 0x1000>;
> +				interrupts = <7>;
> +			};
> +
> +			uart3: uart@0c000 {
> +				compatible = "arm,pl011", "arm,primecell";
> +				reg = <0x0c000 0x1000>;
> +				interrupts = <8>;
> +			};
> +
> +			wdt@0f000 {
> +				compatible = "arm,sp805", "arm,primecell";
> +				reg = <0x0f000 0x1000>;
> +				interrupts = <0>;
> +			};
> +
> +			// Timer init is hardcoded in v2m_timer_init(), for now.
> +			// timer@11000 {
> +			//	compatible = "arm,arm-sp804";

arm,sp804 is more consistent. I believe the sp804 does have the periphid
registers, so arm,primecell should also be added.

> +			//	reg = <0x11000 0x1000>;
> +			//	interrupts = <2>;
> +			// };
> +
> +			// timer@12000 {
> +			//	compatible = "arm,arm-sp804";
> +			//	reg = <0x12000 0x1000>;
> +			// };

Just because Linux is not using it, doesn't mean you should comment it out.

> +
> +			// DVI I2C bus (DDC)
> +			i2c1: i2c@16000 {
> +				compatible = "arm,versatile-i2c";
> +				reg = <0x16000 0x1000>;
> +
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				edid@50 {
> +					compatible = "edid";
> +					reg = <0x50>;
> +				};
> +			};
> +
> +			rtc@17000 {
> +				compatible = "arm,pl031", "arm,primecell";
> +				reg = <0x017000 0x1000>;
> +				interrupts = <4>;
> +			};
> +
> +			compact-flash@1a000 {
> +				compatible = "ata-generic";
> +				reg = <0x1a000 0x100
> +				       0x1a100 0xf00>;
> +				reg-shift = <2>;
> +			};
> +		};
> +	};
> +};
> diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
> new file mode 100644
> index 0000000..059be97
> --- /dev/null
> +++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
> @@ -0,0 +1,80 @@
> +// ARM Ltd. Versatile Express Corex-A9 (Quad Core) Core Tile V2P-CA9 (HBI-0191B)
> +
> +/dts-v1/;
> +
> +/include/ "skeleton.dtsi"
> +
> +/ {
> +	model = "ARM Versatile Express (Cortex-A9 Quad Core Tile)";
> +	compatible = "arm,vexpress-v2p-ca9", "arm,vexpress";
> +	interrupt-parent = <&intc>;
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x60000000 0x40000000>;
> +	};
> +
> +	intc: interrupt-controller@1e001000 {
> +		compatible = "arm,cortex-a9-gic";
> +		#interrupt-cells = <2>;
> +		#address-cells = <0>;
> +		interrupt-controller;
> +		reg = <0x1e001000 0x1000>,
> +		      <0x1e000100 0x100>;
> +	};

Is this really all by itself? It should be in the sub-tree of the
appropriate bus.

You need an "interrupt-parent;" line so the parent is not itself.

> +
> +	motherboard {
> +		ranges = <0 0 0x40000000 0x04000000
> +			  1 0 0x44000000 0x04000000
> +			  2 0 0x48000000 0x04000000
> +			  3 0 0x4c000000 0x04000000
> +			  7 0 0x10000000 0x00020000>;
> +
> +		interrupt-map-mask = <0 0 63>;
> +		interrupt-map = <0 0 0 &intc 32 8
> +				 0 0 1 &intc 33 4
> +				 0 0 2 &intc 34 4
> +				 0 0 3 &intc 35 4
> +				 0 0 4 &intc 36 4
> +				 0 0 5 &intc 37 4
> +				 0 0 6 &intc 38 4
> +				 0 0 7 &intc 39 4
> +				 0 0 8 &intc 40 4
> +				 0 0 9 &intc 41 4
> +				 0 0 10 &intc 42 4
> +				 0 0 11 &intc 43 4
> +				 0 0 12 &intc 44 4
> +				 0 0 13 &intc 45 4
> +				 0 0 14 &intc 46 4
> +				 0 0 15 &intc 47 4
> +				 0 0 16 &intc 48 4
> +				 0 0 17 &intc 49 4
> +				 0 0 18 &intc 50 4
> +				 0 0 19 &intc 51 4
> +				 0 0 20 &intc 52 4
> +				 0 0 21 &intc 53 4
> +				 0 0 22 &intc 54 4
> +				 0 0 23 &intc 55 4
> +				 0 0 24 &intc 56 4
> +				 0 0 25 &intc 57 4
> +				 0 0 26 &intc 58 4
> +				 0 0 27 &intc 59 4
> +				 0 0 28 &intc 60 4
> +				 0 0 29 &intc 61 4
> +				 0 0 30 &intc 62 4
> +				 0 0 31 &intc 63 4
> +				 0 0 32 &intc 64 4
> +				 0 0 33 &intc 65 4
> +				 0 0 34 &intc 66 4
> +				 0 0 35 &intc 67 4
> +				 0 0 36 &intc 68 4
> +				 0 0 37 &intc 69 4
> +				 0 0 38 &intc 70 4
> +				 0 0 39 &intc 71 4
> +				 0 0 40 &intc 72 4
> +				 0 0 41 &intc 73 4
> +				 0 0 42 &intc 74 4>;
> +	};
> +};
> +
> +/include/ "vexpress-v2m-legacy.dtsi"
> diff --git a/arch/arm/configs/vexpress_defconfig b/arch/arm/configs/vexpress_defconfig
> index f2de51f..6c3c5f6 100644
> --- a/arch/arm/configs/vexpress_defconfig
> +++ b/arch/arm/configs/vexpress_defconfig
> @@ -22,6 +22,7 @@ CONFIG_MODULE_UNLOAD=y
>  # CONFIG_IOSCHED_DEADLINE is not set
>  # CONFIG_IOSCHED_CFQ is not set
>  CONFIG_ARCH_VEXPRESS=y
> +CONFIG_ARCH_VEXPRESS_ATAGS=y
>  CONFIG_ARCH_VEXPRESS_CA9X4=y
>  # CONFIG_SWP_EMULATE is not set
>  CONFIG_SMP=y
> diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
> index 9311484..ea64630 100644
> --- a/arch/arm/mach-vexpress/Kconfig
> +++ b/arch/arm/mach-vexpress/Kconfig
> @@ -1,12 +1,55 @@
>  menu "Versatile Express platform type"
>  	depends on ARCH_VEXPRESS
>  
> +# ARCH_VEXPRESS ensures a sane minimal config is selected by selecting
> +# ARCH_VEXPRESS_SANE_CONFIG.
> +# Extend the logic here when adding new core tiles.
> +
> +config ARCH_VEXPRESS_SANE_CONFIG
> +	bool
> +	select ARCH_VEXPRESS_CA9X4
> +	select ARCH_VEXPRESS_ATAGS if !ARCH_VEXPRESS_DT
> +
> +
> +comment "At least one boot type must be selected"
> +
> +config ARCH_VEXPRESS_ATAGS
> +	bool "Boot via ATAGs"
> +	default y
> +	help
> +	  This option enables support for the board using the standard
> +	  ATAGs boot protocol.
> +
> +	  If your bootloader supports FDT-based booting and you do not
> +	  intend ever to boot via the traditional ATAGs method, you can say
> +	  N here.
> +
> +config ARCH_VEXPRESS_DT
> +	bool "Boot via Device Tree"
> +	select USE_OF
> +	help
> +	  This option enables support for the board, and enables booting
> +	  via a Flattened Device Tree provided by the bootloader.
> +
> +	  If your bootloader supports FDT-based booting, you can say Y
> +	  here, otherwise, say N.
> +
> +
> +# Core Tile support options
> +
> +comment "At least one core tile must be selected"
> +
>  config ARCH_VEXPRESS_CA9X4
> -	bool "Versatile Express Cortex-A9x4 tile"
> +	bool "Versatile Express Cortex-A9x4 Core Tile"
> +	default y
>  	select CPU_V7
>  	select ARM_GIC
>  	select ARM_ERRATA_720789
>  	select ARM_ERRATA_751472
>  	select ARM_ERRATA_753970
> +	help
> +	  Include support for the Cortex-A9x4 Core Tile (HBI-0191B).
> +
> +	  If unsure, say Y.
>  
>  endmenu
> diff --git a/arch/arm/mach-vexpress/ct-ca9x4.c b/arch/arm/mach-vexpress/ct-ca9x4.c
> index bfd32f5..e2fe2c9 100644
> --- a/arch/arm/mach-vexpress/ct-ca9x4.c
> +++ b/arch/arm/mach-vexpress/ct-ca9x4.c
> @@ -9,6 +9,7 @@
>  #include <linux/amba/bus.h>
>  #include <linux/amba/clcd.h>
>  #include <linux/clkdev.h>
> +#include <linux/irqdomain.h>
>  
>  #include <asm/hardware/arm_timer.h>
>  #include <asm/hardware/cache-l2x0.h>
> @@ -59,10 +60,16 @@ static void __init ct_ca9x4_map_io(void)
>  	iotable_init(ct_ca9x4_io_desc, ARRAY_SIZE(ct_ca9x4_io_desc));
>  }
>  
> +static const struct of_device_id gic_of_match[] __initconst = {
> +	{ .compatible = "arm,cortex-a9-gic", },
> +	{}
> +};
> +
>  static void __init ct_ca9x4_init_irq(void)
>  {
>  	gic_init(0, 29, MMIO_P2V(A9_MPCORE_GIC_DIST),
>  		 MMIO_P2V(A9_MPCORE_GIC_CPU));
> +	irq_domain_generate_simple(gic_of_match, A9_MPCORE_GIC_DIST, 0);
>  }
>  
>  #if 0
> diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
> index 9e6b93b..6defce6 100644
> --- a/arch/arm/mach-vexpress/v2m.c
> +++ b/arch/arm/mach-vexpress/v2m.c
> @@ -6,6 +6,8 @@
>  #include <linux/amba/mmci.h>
>  #include <linux/io.h>
>  #include <linux/init.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/ata_platform.h>
>  #include <linux/smsc911x.h>
> @@ -118,7 +120,7 @@ int v2m_cfg_read(u32 devfn, u32 *data)
>  	return !!(val & SYS_CFG_ERR);
>  }
>  
> -
> +#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
>  static struct resource v2m_pcie_i2c_resource = {
>  	.start	= V2M_SERIAL_BUS_PCI,
>  	.end	= V2M_SERIAL_BUS_PCI + SZ_4K - 1,
> @@ -200,6 +202,7 @@ static struct platform_device v2m_usb_device = {
>  	.num_resources	= ARRAY_SIZE(v2m_usb_resources),
>  	.dev.platform_data = &v2m_usb_config,
>  };
> +#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
>  
>  static void v2m_flash_set_vpp(struct platform_device *pdev, int on)
>  {
> @@ -211,6 +214,7 @@ static struct physmap_flash_data v2m_flash_data = {
>  	.set_vpp	= v2m_flash_set_vpp,
>  };
>  
> +#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
>  static struct resource v2m_flash_resources[] = {
>  	{
>  		.start	= V2M_NOR0,
> @@ -254,6 +258,7 @@ static struct platform_device v2m_cf_device = {
>  	.num_resources	= ARRAY_SIZE(v2m_pata_resources),
>  	.dev.platform_data = &v2m_pata_data,
>  };
> +#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
>  
>  static unsigned int v2m_mmci_status(struct device *dev)
>  {
> @@ -265,6 +270,7 @@ static struct mmci_platform_data v2m_mmci_data = {
>  	.status		= v2m_mmci_status,
>  };
>  
> +#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
>  static AMBA_DEVICE(aaci,  "mb:aaci",  V2M_AACI, NULL);
>  static AMBA_DEVICE(mmci,  "mb:mmci",  V2M_MMCI, &v2m_mmci_data);
>  static AMBA_DEVICE(kmi0,  "mb:kmi0",  V2M_KMI0, NULL);
> @@ -288,6 +294,7 @@ static struct amba_device *v2m_amba_devs[] __initdata = {
>  	&wdt_device,
>  	&rtc_device,
>  };
> +#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
>  
>  
>  static long v2m_osc_round(struct clk *clk, unsigned long rate)
> @@ -415,6 +422,8 @@ static void __init v2m_init_irq(void)
>  	ct_desc->init_irq();
>  }
>  
> +
> +#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
>  static void __init v2m_init(void)
>  {
>  	int i;
> @@ -443,3 +452,46 @@ MACHINE_START(VEXPRESS, "ARM-Versatile Express")
>  	.timer		= &v2m_timer,
>  	.init_machine	= v2m_init,
>  MACHINE_END
> +#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
> +
> +#ifdef CONFIG_ARCH_VEXPRESS_DT
> +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_AACI, "mb:aaci", NULL),
> +	OF_DEV_AUXDATA("arm,primecell", V2M_WDT, "mb:wdt", NULL),
> +	OF_DEV_AUXDATA("arm,primecell", V2M_MMCI, "mb:mmci", &v2m_mmci_data),
> +	OF_DEV_AUXDATA("arm,primecell", V2M_KMI0, "mb:kmi0", NULL),
> +	OF_DEV_AUXDATA("arm,primecell", V2M_KMI1, "mb:kmi1", NULL),
> +	OF_DEV_AUXDATA("arm,primecell", V2M_UART0, "mb:uart0", NULL),
> +	OF_DEV_AUXDATA("arm,primecell", V2M_UART1, "mb:uart1", NULL),
> +	OF_DEV_AUXDATA("arm,primecell", V2M_UART2, "mb:uart2", NULL),
> +	OF_DEV_AUXDATA("arm,primecell", V2M_UART3, "mb:uart3", NULL),
> +	OF_DEV_AUXDATA("arm,primecell", V2M_RTC, "mb:rtc", NULL),
> +	{}
> +};
> +
> +static void __init v2m_dt_init(void)
> +{
> +	of_platform_populate(NULL, of_default_bus_match_table,
> +			     v2m_dt_auxdata_lookup, NULL);
> +
> +	pm_power_off = v2m_power_off;
> +	arm_pm_restart = v2m_restart;
> +
> +	ct_desc->init_tile();
> +}
> +
> +static const char *v2m_dt_match[] __initconst = {
> +	"arm,vexpress",
> +	NULL,
> +};
> +
> +DT_MACHINE_START(VEXPRESS_DT, "ARM Versatile Express")
> +	.map_io		= v2m_map_io,
> +	.init_early	= v2m_init_early,
> +	.init_irq	= v2m_init_irq,
> +	.timer		= &v2m_timer,
> +	.init_machine	= v2m_dt_init,
> +	.dt_compat	= v2m_dt_match,
> +MACHINE_END
> +#endif /* CONFIG_ARCH_VEXPRESS_DT */

All the ifdefs are really ugly. Most people are creating new board_dt.c
file and copying over pieces they need. Once DT support is on par with
the old file, the old file can be deleted.

Rob
Dave Martin - Sept. 21, 2011, 2:24 p.m.
On Wed, Sep 21, 2011 at 08:24:24AM -0500, Rob Herring wrote:
> Dave,
> 
> On 09/21/2011 04:19 AM, Dave Martin wrote:
> > This patch implements initial support for booting using a flattened
> > device tree on the Versatile Express platform.
> > 
> > Eventually, it should be possible to present a single, core-tile-
> > independent board, but in this transitional patch the baseboard +
> > Cortex-A9x4 core tile combination is the only directly supported
> > platform, since the implementation is not yet fully generic.
> > 
> > For now, clocks and timers are not handled via the device tree.
> > Implementation of these can follow in later patches.
> > 
> > Thanks to Lorenzo Pieralisi, Grant Likely and Paweł Moll for their
> > help and contributions.
> > 
> > Signed-off-by: Dave Martin <dave.martin@linaro.org>
> > Acked-by: Paweł Moll <Pawel.Moll@arm.com>
> > ---
> > 
> > There are some outstanding issues which need to be discussed, listed
> > below.
> > 
> >   * This patch is not currently based on the GIC bindings being
> >     discussed by Rob Herring et al.  Once that discussion reaches a
> >     conclusion, it should be straightforward to rebase onto the result.
> > 
> >   * The following added bindings are not present upstream and need
> >     documentation / discussion:
> > 
> >       * arm,vexpress -- the global board binding for all platform
> >         combinations using the Versatile Express motherboard.
> > 
> >       * arm,vexpress-v2p-ca9 -- the specific binding for the Versatile
> >         Express motherboard with Cortex-A9x4 core tile installed.  It
> >         is only mentioned as the most-specific match in vexpress-v2p-
> >         ca9.dts
> > 
> >         Since it's intended that the motherboard code should be fully
> >         generic, and because no other core tiles are upstream yet,
> >         perhaps we can get rid of this binding right away.
> > 
> >       * edid -- It should be possible to have a fairly generic binding
> >         for EDID interfaces, but none seems to exist yet.  Discussion
> >         is needed regarding what form this should take.
> > 
> >         This might more appropriately be called "ddc" (or some
> >         variation on that), since EDID seems only to describe the
> >         format of the ID data retrievable via this interface; not the
> >         interface itself.
> > 
> >       * arm,vexpress-flash -- Needed because of the requirement to
> >         provide the physmap_flash driver with a special .set_vpp
> >         handler.
> > 
> >       * idt,89hpes32h8 -- This is the IDT 89HPES32H8 PCI express
> >         interconnect switch.  This isn't needed for the Versatile
> >         Express to work, but would be needed if using PCI-e peripherals
> >         for real.  I expect that more driver support needs to go
> >         upstream before this is actually usable.
> > 
> >       * nxp,isp1761 -- The driver support for this is already upstream
> >         (with some minor issues for ARM support).
> > 
> >       * arm,amba-bus -- widely used by other boards and patchsets, but
> >         seems not to be documented.
> > 
> 
> This should be dropped. There's not really any bus component to an amba
> bus. All the probing info is within the primecell peripherals.

So, just use "simple-bus"?

> >       * The following bindings for ARM primecell peripherals are used
> >         elsewhere but not documented.  They should be pretty simple and
> >         uncontraversial.
> >           * arm,pl031
> >           * arm,pl041
> >           * arm,pl050
> >           * arm,pl180
> >           * arm,sp805
> > 
> 
> Plus pl011, pl010, sp804, pl022, pl061

It looks like I missed pl011 and sp804 (though I don't currently declare
the timers in the device tree because of the way they are initialised).

> >         Rob Herring suggested documenting simple bindings for these
> >         (and others) along with his initial amba device tree probe
> >         patches, but these bindings don't seem to be documented
> >         upstream for now.
> > 
> 
> pl330 went the other route with a file for itself. That may be better to
> avoid conflicts. But yes, ARM should document all their peripherals. ;)
> 
> I'll do the ones on highbank if you want to do the rest on VExp.

OK, I'll try to propose documentation for these:

	* arm,pl011
	* arm,pl031
	* arm,pl041
	* arm,pl050
	* arm,pl180
	* arm,sp804
	* arm,sp805

...if you can pick the other ones that are relevant to highbank -- thanks.

> 
> > 
> >   * Shawn Guo's smsc911x patch is needed for Ethernet to work.  This is
> >     headed upstream but not yet in mainline.  It is available in -next.
> > 
> >   * Minor patches are needed to the isp1760 and pata_generic drivers,
> >     to allow OF-based initialisation across a wider group of
> >     architectures.  These are being discussed independently, but are
> >     not yet accepted for merging upstream.
> > 
> >   * Most core-tile peripherals are currently not described in the core-
> >     tile device tree fragment.  This is a lower-priority issue since
> >     the motherboard code already autodetects the core-tile (though only
> >     one core-tile is fully upstream at the moment).
> > 
> >   * Static peripheral mappings are not yet handled in a generic way in
> >     the board support code.  This is a prerequisite for supporting
> >     multiple core-tiles int the same kernel.  It well need to get fixed
> >     later, when extra core tile support is merged (or before).
> > 
> >     Paweł Moll is looking into this separately.
> > 
> >   * The Kconfig logic for ensuring that at least one boot protocol and
> >     at least one core tile are selected is a bit ugly.  Suggestions for
> >     improving this are certainly welcome.
> > 
> >  arch/arm/Kconfig                           |    1 +
> >  arch/arm/boot/dts/vexpress-v2m-legacy.dtsi |  163 ++++++++++++++++++++++++++++
> >  arch/arm/boot/dts/vexpress-v2p-ca9.dts     |   80 ++++++++++++++
> >  arch/arm/configs/vexpress_defconfig        |    1 +
> >  arch/arm/mach-vexpress/Kconfig             |   45 ++++++++-
> >  arch/arm/mach-vexpress/ct-ca9x4.c          |    7 ++
> >  arch/arm/mach-vexpress/v2m.c               |   54 +++++++++-
> >  7 files changed, 349 insertions(+), 2 deletions(-)
> >  create mode 100644 arch/arm/boot/dts/vexpress-v2m-legacy.dtsi
> >  create mode 100644 arch/arm/boot/dts/vexpress-v2p-ca9.dts
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 5ebc5d9..a6e90d5 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -282,6 +282,7 @@ config ARCH_VERSATILE
> >  
> >  config ARCH_VEXPRESS
> >  	bool "ARM Ltd. Versatile Express family"
> > +	select ARCH_VEXPRESS_SANE_CONFIG
> >  	select ARCH_WANT_OPTIONAL_GPIOLIB
> >  	select ARM_AMBA
> >  	select ARM_TIMER_SP804
> > diff --git a/arch/arm/boot/dts/vexpress-v2m-legacy.dtsi b/arch/arm/boot/dts/vexpress-v2m-legacy.dtsi
> > new file mode 100644
> > index 0000000..fd6e4e4
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/vexpress-v2m-legacy.dtsi
> > @@ -0,0 +1,163 @@
> > +// ARM Ltd. Versatile Express Motherboard V2M-P1 (HBI-0190D)
> > +// Legacy memory map
> 
> Not sure, but C++ style comments are probably frowned upon in dts too.
> 
> > +
> > +/ {
> > +	aliases {
> > +		serial0 = &uart0;
> > +		serial1 = &uart1;
> > +		serial2 = &uart2;
> > +		serial3 = &uart3;
> > +		i2c0 = &i2c0;
> > +		i2c1 = &i2c1;
> > +	};
> > +
> > +	motherboard {
> > +		compatible = "simple-bus";
> > +		#address-cells = <2>; // SMB chipselect number and offset
> > +		#size-cells = <1>;
> > +		#interrupt-cells = <1>;
> > +
> > +		flash@0,00000000 {
> > +			compatible = "arm,vexpress-flash", "cfi-flash";
> > +			reg = <0 0x00000000 0x04000000
> > +			       1 0x00000000 0x04000000>;
> > +			bank-width = <4>;
> > +		};
> > +
> > +		psram@2,00000000 {
> > +			compatible = "mtd-ram";
> > +			reg = <2 0x00000000 0x02000000>;
> > +			bank-width = <4>;
> > +		};
> > +
> > +		ethernet@3,02000000 {
> > +			compatible = "smsc,lan9118", "smsc,lan9115";
> > +			reg = <3 0x02000000 0x10000>;
> > +			reg-io-width = <4>;
> > +			interrupts = <15>;
> > +			smsc,irq-active-high;
> > +			smsc,irq-push-pull;
> > +		};
> > +
> > +		usb@3,03000000 {
> > +			compatible = "nxp,usb-isp1761";
> > +			reg = <3 0x03000000 0x20000>;
> > +			interrupts = <16>;
> > +			port1-otg;
> > +		};
> > +
> > +		peripherals@7,00000000 {
> > +			compatible = "arm,amba-bus", "simple-bus";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			ranges = <0 7 0 0x20000>;
> > +
> > +			// PCI-E I2C bus
> > +			i2c0: i2c@02000 {
> > +				compatible = "arm,versatile-i2c";
> > +				reg = <0x02000 0x1000>;
> > +
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				pcie-switch@60 {
> > +					compatible = "idt,89hpes32h8";
> > +					reg = <0x60>;
> > +				};
> > +			};
> > +
> > +			aaci@04000 {
> > +				compatible = "arm,pl041", "arm,primecell";
> > +				reg = <0x04000 0x1000>;
> > +				interrupts = <11>;
> > +			};
> > +
> > +			mmci@05000 {
> > +				compatible = "arm,pl180", "arm,primecell";
> > +				reg = <0x05000 0x1000>;
> > +				interrupts = <9 10>;
> > +			};
> > +
> > +			kmi@06000 {
> > +				compatible = "arm,pl050", "arm,primecell";
> > +				reg = <0x06000 0x1000>;
> > +				interrupts = <12>;
> > +			};
> > +
> > +			kmi@07000 {
> > +				compatible = "arm,pl050", "arm,primecell";
> > +				reg = <0x07000 0x1000>;
> > +				interrupts = <13>;
> > +			};
> > +
> > +			uart0: uart@09000 {
> > +				compatible = "arm,pl011", "arm,primecell";
> > +				reg = <0x09000 0x1000>;
> > +				interrupts = <5>;
> > +			};
> > +
> > +			uart1: uart@0a000 {
> > +				compatible = "arm,pl011", "arm,primecell";
> > +				reg = <0x0a000 0x1000>;
> > +				interrupts = <6>;
> > +			};
> > +
> > +			uart2: uart@0b000 {
> > +				compatible = "arm,pl011", "arm,primecell";
> > +				reg = <0x0b000 0x1000>;
> > +				interrupts = <7>;
> > +			};
> > +
> > +			uart3: uart@0c000 {
> > +				compatible = "arm,pl011", "arm,primecell";
> > +				reg = <0x0c000 0x1000>;
> > +				interrupts = <8>;
> > +			};
> > +
> > +			wdt@0f000 {
> > +				compatible = "arm,sp805", "arm,primecell";
> > +				reg = <0x0f000 0x1000>;
> > +				interrupts = <0>;
> > +			};
> > +
> > +			// Timer init is hardcoded in v2m_timer_init(), for now.
> > +			// timer@11000 {
> > +			//	compatible = "arm,arm-sp804";
> 
> arm,sp804 is more consistent. I believe the sp804 does have the periphid
> registers, so arm,primecell should also be added.

Do you mean "does not have"?  If so, the periphid will be needed -- thanks for
pointing it out in that case.

I will make the names consistent.  These were pasted from someone Lorenzo's
older patches, and failed to sport e the inconsistency since I wasn't
actually making use of these entries yet.

> > +			//	reg = <0x11000 0x1000>;
> > +			//	interrupts = <2>;
> > +			// };
> > +
> > +			// timer@12000 {
> > +			//	compatible = "arm,arm-sp804";
> > +			//	reg = <0x12000 0x1000>;
> > +			// };
> 
> Just because Linux is not using it, doesn't mean you should comment it out.

From the point of view of describing the hardware, yes.  However, I was
a bit worried that if sp804 is turned into a full driver, it will get
initialised twice -- once explicitly and once in of_platform_populate()...
at least until the baord code is adapted to work properly with the new
driver.

Commenting these entries out for now seemed a good idea to avoid the flag-day
hazard.  Am I being too cautious?

> 
> > +
> > +			// DVI I2C bus (DDC)
> > +			i2c1: i2c@16000 {
> > +				compatible = "arm,versatile-i2c";
> > +				reg = <0x16000 0x1000>;
> > +
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				edid@50 {
> > +					compatible = "edid";
> > +					reg = <0x50>;
> > +				};
> > +			};
> > +
> > +			rtc@17000 {
> > +				compatible = "arm,pl031", "arm,primecell";
> > +				reg = <0x017000 0x1000>;
> > +				interrupts = <4>;
> > +			};
> > +
> > +			compact-flash@1a000 {
> > +				compatible = "ata-generic";
> > +				reg = <0x1a000 0x100
> > +				       0x1a100 0xf00>;
> > +				reg-shift = <2>;
> > +			};
> > +		};
> > +	};
> > +};
> > diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
> > new file mode 100644
> > index 0000000..059be97
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
> > @@ -0,0 +1,80 @@
> > +// ARM Ltd. Versatile Express Corex-A9 (Quad Core) Core Tile V2P-CA9 (HBI-0191B)
> > +
> > +/dts-v1/;
> > +
> > +/include/ "skeleton.dtsi"
> > +
> > +/ {
> > +	model = "ARM Versatile Express (Cortex-A9 Quad Core Tile)";
> > +	compatible = "arm,vexpress-v2p-ca9", "arm,vexpress";
> > +	interrupt-parent = <&intc>;
> > +
> > +	memory {
> > +		device_type = "memory";
> > +		reg = <0x60000000 0x40000000>;
> > +	};
> > +
> > +	intc: interrupt-controller@1e001000 {
> > +		compatible = "arm,cortex-a9-gic";
> > +		#interrupt-cells = <2>;
> > +		#address-cells = <0>;
> > +		interrupt-controller;
> > +		reg = <0x1e001000 0x1000>,
> > +		      <0x1e000100 0x100>;
> > +	};
> 
> Is this really all by itself? It should be in the sub-tree of the
> appropriate bus.

Hmmm, yes.  I guess I got away with this due to not using the proper GIC
bindings yet (and not declaring the other core-tile peripherals).

> You need an "interrupt-parent;" line so the parent is not itself.

Do you mean for the bus?

> 
> > +
> > +	motherboard {
> > +		ranges = <0 0 0x40000000 0x04000000
> > +			  1 0 0x44000000 0x04000000
> > +			  2 0 0x48000000 0x04000000
> > +			  3 0 0x4c000000 0x04000000
> > +			  7 0 0x10000000 0x00020000>;
> > +
> > +		interrupt-map-mask = <0 0 63>;
> > +		interrupt-map = <0 0 0 &intc 32 8

^ This should be ... 4 btw (thanks to Pawel for spotting that)

> > +				 0 0 1 &intc 33 4
> > +				 0 0 2 &intc 34 4
> > +				 0 0 3 &intc 35 4
> > +				 0 0 4 &intc 36 4
> > +				 0 0 5 &intc 37 4
> > +				 0 0 6 &intc 38 4
> > +				 0 0 7 &intc 39 4
> > +				 0 0 8 &intc 40 4
> > +				 0 0 9 &intc 41 4
> > +				 0 0 10 &intc 42 4
> > +				 0 0 11 &intc 43 4
> > +				 0 0 12 &intc 44 4
> > +				 0 0 13 &intc 45 4
> > +				 0 0 14 &intc 46 4
> > +				 0 0 15 &intc 47 4
> > +				 0 0 16 &intc 48 4
> > +				 0 0 17 &intc 49 4
> > +				 0 0 18 &intc 50 4
> > +				 0 0 19 &intc 51 4
> > +				 0 0 20 &intc 52 4
> > +				 0 0 21 &intc 53 4
> > +				 0 0 22 &intc 54 4
> > +				 0 0 23 &intc 55 4
> > +				 0 0 24 &intc 56 4
> > +				 0 0 25 &intc 57 4
> > +				 0 0 26 &intc 58 4
> > +				 0 0 27 &intc 59 4
> > +				 0 0 28 &intc 60 4
> > +				 0 0 29 &intc 61 4
> > +				 0 0 30 &intc 62 4
> > +				 0 0 31 &intc 63 4
> > +				 0 0 32 &intc 64 4
> > +				 0 0 33 &intc 65 4
> > +				 0 0 34 &intc 66 4
> > +				 0 0 35 &intc 67 4
> > +				 0 0 36 &intc 68 4
> > +				 0 0 37 &intc 69 4
> > +				 0 0 38 &intc 70 4
> > +				 0 0 39 &intc 71 4
> > +				 0 0 40 &intc 72 4
> > +				 0 0 41 &intc 73 4
> > +				 0 0 42 &intc 74 4>;
> > +	};
> > +};
> > +
> > +/include/ "vexpress-v2m-legacy.dtsi"
> > diff --git a/arch/arm/configs/vexpress_defconfig b/arch/arm/configs/vexpress_defconfig
> > index f2de51f..6c3c5f6 100644
> > --- a/arch/arm/configs/vexpress_defconfig
> > +++ b/arch/arm/configs/vexpress_defconfig
> > @@ -22,6 +22,7 @@ CONFIG_MODULE_UNLOAD=y
> >  # CONFIG_IOSCHED_DEADLINE is not set
> >  # CONFIG_IOSCHED_CFQ is not set
> >  CONFIG_ARCH_VEXPRESS=y
> > +CONFIG_ARCH_VEXPRESS_ATAGS=y
> >  CONFIG_ARCH_VEXPRESS_CA9X4=y
> >  # CONFIG_SWP_EMULATE is not set
> >  CONFIG_SMP=y
> > diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
> > index 9311484..ea64630 100644
> > --- a/arch/arm/mach-vexpress/Kconfig
> > +++ b/arch/arm/mach-vexpress/Kconfig
> > @@ -1,12 +1,55 @@
> >  menu "Versatile Express platform type"
> >  	depends on ARCH_VEXPRESS
> >  
> > +# ARCH_VEXPRESS ensures a sane minimal config is selected by selecting
> > +# ARCH_VEXPRESS_SANE_CONFIG.
> > +# Extend the logic here when adding new core tiles.
> > +
> > +config ARCH_VEXPRESS_SANE_CONFIG
> > +	bool
> > +	select ARCH_VEXPRESS_CA9X4
> > +	select ARCH_VEXPRESS_ATAGS if !ARCH_VEXPRESS_DT
> > +
> > +
> > +comment "At least one boot type must be selected"
> > +
> > +config ARCH_VEXPRESS_ATAGS
> > +	bool "Boot via ATAGs"
> > +	default y
> > +	help
> > +	  This option enables support for the board using the standard
> > +	  ATAGs boot protocol.
> > +
> > +	  If your bootloader supports FDT-based booting and you do not
> > +	  intend ever to boot via the traditional ATAGs method, you can say
> > +	  N here.
> > +
> > +config ARCH_VEXPRESS_DT
> > +	bool "Boot via Device Tree"
> > +	select USE_OF
> > +	help
> > +	  This option enables support for the board, and enables booting
> > +	  via a Flattened Device Tree provided by the bootloader.
> > +
> > +	  If your bootloader supports FDT-based booting, you can say Y
> > +	  here, otherwise, say N.
> > +
> > +
> > +# Core Tile support options
> > +
> > +comment "At least one core tile must be selected"
> > +
> >  config ARCH_VEXPRESS_CA9X4
> > -	bool "Versatile Express Cortex-A9x4 tile"
> > +	bool "Versatile Express Cortex-A9x4 Core Tile"
> > +	default y
> >  	select CPU_V7
> >  	select ARM_GIC
> >  	select ARM_ERRATA_720789
> >  	select ARM_ERRATA_751472
> >  	select ARM_ERRATA_753970
> > +	help
> > +	  Include support for the Cortex-A9x4 Core Tile (HBI-0191B).
> > +
> > +	  If unsure, say Y.
> >  
> >  endmenu
> > diff --git a/arch/arm/mach-vexpress/ct-ca9x4.c b/arch/arm/mach-vexpress/ct-ca9x4.c
> > index bfd32f5..e2fe2c9 100644
> > --- a/arch/arm/mach-vexpress/ct-ca9x4.c
> > +++ b/arch/arm/mach-vexpress/ct-ca9x4.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/amba/bus.h>
> >  #include <linux/amba/clcd.h>
> >  #include <linux/clkdev.h>
> > +#include <linux/irqdomain.h>
> >  
> >  #include <asm/hardware/arm_timer.h>
> >  #include <asm/hardware/cache-l2x0.h>
> > @@ -59,10 +60,16 @@ static void __init ct_ca9x4_map_io(void)
> >  	iotable_init(ct_ca9x4_io_desc, ARRAY_SIZE(ct_ca9x4_io_desc));
> >  }
> >  
> > +static const struct of_device_id gic_of_match[] __initconst = {
> > +	{ .compatible = "arm,cortex-a9-gic", },
> > +	{}
> > +};
> > +
> >  static void __init ct_ca9x4_init_irq(void)
> >  {
> >  	gic_init(0, 29, MMIO_P2V(A9_MPCORE_GIC_DIST),
> >  		 MMIO_P2V(A9_MPCORE_GIC_CPU));
> > +	irq_domain_generate_simple(gic_of_match, A9_MPCORE_GIC_DIST, 0);
> >  }
> >  
> >  #if 0
> > diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
> > index 9e6b93b..6defce6 100644
> > --- a/arch/arm/mach-vexpress/v2m.c
> > +++ b/arch/arm/mach-vexpress/v2m.c
> > @@ -6,6 +6,8 @@
> >  #include <linux/amba/mmci.h>
> >  #include <linux/io.h>
> >  #include <linux/init.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/ata_platform.h>
> >  #include <linux/smsc911x.h>
> > @@ -118,7 +120,7 @@ int v2m_cfg_read(u32 devfn, u32 *data)
> >  	return !!(val & SYS_CFG_ERR);
> >  }
> >  
> > -
> > +#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
> >  static struct resource v2m_pcie_i2c_resource = {
> >  	.start	= V2M_SERIAL_BUS_PCI,
> >  	.end	= V2M_SERIAL_BUS_PCI + SZ_4K - 1,
> > @@ -200,6 +202,7 @@ static struct platform_device v2m_usb_device = {
> >  	.num_resources	= ARRAY_SIZE(v2m_usb_resources),
> >  	.dev.platform_data = &v2m_usb_config,
> >  };
> > +#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
> >  
> >  static void v2m_flash_set_vpp(struct platform_device *pdev, int on)
> >  {
> > @@ -211,6 +214,7 @@ static struct physmap_flash_data v2m_flash_data = {
> >  	.set_vpp	= v2m_flash_set_vpp,
> >  };
> >  
> > +#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
> >  static struct resource v2m_flash_resources[] = {
> >  	{
> >  		.start	= V2M_NOR0,
> > @@ -254,6 +258,7 @@ static struct platform_device v2m_cf_device = {
> >  	.num_resources	= ARRAY_SIZE(v2m_pata_resources),
> >  	.dev.platform_data = &v2m_pata_data,
> >  };
> > +#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
> >  
> >  static unsigned int v2m_mmci_status(struct device *dev)
> >  {
> > @@ -265,6 +270,7 @@ static struct mmci_platform_data v2m_mmci_data = {
> >  	.status		= v2m_mmci_status,
> >  };
> >  
> > +#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
> >  static AMBA_DEVICE(aaci,  "mb:aaci",  V2M_AACI, NULL);
> >  static AMBA_DEVICE(mmci,  "mb:mmci",  V2M_MMCI, &v2m_mmci_data);
> >  static AMBA_DEVICE(kmi0,  "mb:kmi0",  V2M_KMI0, NULL);
> > @@ -288,6 +294,7 @@ static struct amba_device *v2m_amba_devs[] __initdata = {
> >  	&wdt_device,
> >  	&rtc_device,
> >  };
> > +#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
> >  
> >  
> >  static long v2m_osc_round(struct clk *clk, unsigned long rate)
> > @@ -415,6 +422,8 @@ static void __init v2m_init_irq(void)
> >  	ct_desc->init_irq();
> >  }
> >  
> > +
> > +#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
> >  static void __init v2m_init(void)
> >  {
> >  	int i;
> > @@ -443,3 +452,46 @@ MACHINE_START(VEXPRESS, "ARM-Versatile Express")
> >  	.timer		= &v2m_timer,
> >  	.init_machine	= v2m_init,
> >  MACHINE_END
> > +#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
> > +
> > +#ifdef CONFIG_ARCH_VEXPRESS_DT
> > +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_AACI, "mb:aaci", NULL),
> > +	OF_DEV_AUXDATA("arm,primecell", V2M_WDT, "mb:wdt", NULL),
> > +	OF_DEV_AUXDATA("arm,primecell", V2M_MMCI, "mb:mmci", &v2m_mmci_data),
> > +	OF_DEV_AUXDATA("arm,primecell", V2M_KMI0, "mb:kmi0", NULL),
> > +	OF_DEV_AUXDATA("arm,primecell", V2M_KMI1, "mb:kmi1", NULL),
> > +	OF_DEV_AUXDATA("arm,primecell", V2M_UART0, "mb:uart0", NULL),
> > +	OF_DEV_AUXDATA("arm,primecell", V2M_UART1, "mb:uart1", NULL),
> > +	OF_DEV_AUXDATA("arm,primecell", V2M_UART2, "mb:uart2", NULL),
> > +	OF_DEV_AUXDATA("arm,primecell", V2M_UART3, "mb:uart3", NULL),
> > +	OF_DEV_AUXDATA("arm,primecell", V2M_RTC, "mb:rtc", NULL),
> > +	{}
> > +};
> > +
> > +static void __init v2m_dt_init(void)
> > +{
> > +	of_platform_populate(NULL, of_default_bus_match_table,
> > +			     v2m_dt_auxdata_lookup, NULL);
> > +
> > +	pm_power_off = v2m_power_off;
> > +	arm_pm_restart = v2m_restart;
> > +
> > +	ct_desc->init_tile();
> > +}
> > +
> > +static const char *v2m_dt_match[] __initconst = {
> > +	"arm,vexpress",
> > +	NULL,
> > +};
> > +
> > +DT_MACHINE_START(VEXPRESS_DT, "ARM Versatile Express")
> > +	.map_io		= v2m_map_io,
> > +	.init_early	= v2m_init_early,
> > +	.init_irq	= v2m_init_irq,
> > +	.timer		= &v2m_timer,
> > +	.init_machine	= v2m_dt_init,
> > +	.dt_compat	= v2m_dt_match,
> > +MACHINE_END
> > +#endif /* CONFIG_ARCH_VEXPRESS_DT */
> 
> All the ifdefs are really ugly. Most people are creating new board_dt.c
> file and copying over pieces they need. Once DT support is on par with
> the old file, the old file can be deleted.

Would you expect the common code between the DT and non-DT boards to
dwindle away to nothing over time?  I wasn't sure whether we would
get that far.

Agreed regarding the ifdefs -- but I thought it would be better to do
this refactoring in a separate patch which _only_ does the refactoring,
once the functional changes are agreed.

That way the actual functional changes will be clear in the history.
If I try to refactor it ahead of time, I will probably miss some
bits of factoring which turn out to be necessary later, resulting
in extra churn.


If this round of review produces something which feels likely to be
close to the final form, I could propose a refactoring patch to go
on top of it, but I'm wary of doing this prematurely.

Cheers
---Dave
Pawel Moll - Sept. 21, 2011, 2:33 p.m.
> OK, I'll try to propose documentation for these:
>         * arm,pl180

You can skip this one - I'll add the description together with the MMCI
driver bindings (it will be 180 and 181, by the way :-)

> > > +			// Timer init is hardcoded in v2m_timer_init(), for now.
> > > +			// timer@11000 {
> > > +			//	compatible = "arm,arm-sp804";
> > 
> > arm,sp804 is more consistent. I believe the sp804 does have the periphid
> > registers, so arm,primecell should also be added.
> 
> Do you mean "does not have"?  If so, the periphid will be needed -- thanks for
> pointing it out in that case.

I think Rob meant it should be
	compatible = "arm,sp804", "arm,primecell",
as SP804 contains the PrimeCell periphid registers, so will be
recognized by amba bus driver.

> I will make the names consistent.  These were pasted from someone Lorenzo's
> older patches, and failed to sport e the inconsistency since I wasn't
> actually making use of these entries yet.
> 
> > > +			//	reg = <0x11000 0x1000>;
> > > +			//	interrupts = <2>;
> > > +			// };
> > > +
> > > +			// timer@12000 {
> > > +			//	compatible = "arm,arm-sp804";
> > > +			//	reg = <0x12000 0x1000>;
> > > +			// };
> > 
> > Just because Linux is not using it, doesn't mean you should comment it out.
> 
> From the point of view of describing the hardware, yes.  However, I was
> a bit worried that if sp804 is turned into a full driver, it will get
> initialised twice -- once explicitly and once in of_platform_populate()...
> at least until the baord code is adapted to work properly with the new
> driver.
> 
> Commenting these entries out for now seemed a good idea to avoid the flag-day
> hazard.  Am I being too cautious?

I think you are ;-) Besides my static-mapping-rework is already using
those...

Cheers!

Paweł
Grant Likely - Sept. 21, 2011, 2:57 p.m.
On Wed, Sep 21, 2011 at 7:24 AM, Rob Herring <robherring2@gmail.com> wrote:
> On 09/21/2011 04:19 AM, Dave Martin wrote:
>>       * arm,amba-bus -- widely used by other boards and patchsets, but
>>         seems not to be documented.
>>
>
> This should be dropped. There's not really any bus component to an amba
> bus. All the probing info is within the primecell peripherals.

No, if it is an AMBA bus, then it is entirely appropriate to declare
it as an amba bus, but to also be compatible with "simple-bus".  In
fact, it would be better to use a compatible string that specifies the
specific implementation of AMBA bus since there are several versions
of the spec.

Don't let Linux's current implementation detail derail what is
considered good practice.

g.
Dave Martin - Sept. 21, 2011, 3:49 p.m.
On Wed, Sep 21, 2011 at 03:33:10PM +0100, Pawel Moll wrote:
> > OK, I'll try to propose documentation for these:
> >         * arm,pl180
> 
> You can skip this one - I'll add the description together with the MMCI
> driver bindings (it will be 180 and 181, by the way :-)

Done.

> > > > +			// Timer init is hardcoded in v2m_timer_init(), for now.
> > > > +			// timer@11000 {
> > > > +			//	compatible = "arm,arm-sp804";
> > > 
> > > arm,sp804 is more consistent. I believe the sp804 does have the periphid
> > > registers, so arm,primecell should also be added.
> > 
> > Do you mean "does not have"?  If so, the periphid will be needed -- thanks for
> > pointing it out in that case.
> 
> I think Rob meant it should be
> 	compatible = "arm,sp804", "arm,primecell",
> as SP804 contains the PrimeCell periphid registers, so will be
> recognized by amba bus driver.

Oh, right -- misunderstanding, sorry for that.

> > I will make the names consistent.  These were pasted from someone Lorenzo's
> > older patches, and failed to sport e the inconsistency since I wasn't
> > actually making use of these entries yet.
> > 
> > > > +			//	reg = <0x11000 0x1000>;
> > > > +			//	interrupts = <2>;
> > > > +			// };
> > > > +
> > > > +			// timer@12000 {
> > > > +			//	compatible = "arm,arm-sp804";
> > > > +			//	reg = <0x12000 0x1000>;
> > > > +			// };
> > > 
> > > Just because Linux is not using it, doesn't mean you should comment it out.
> > 
> > From the point of view of describing the hardware, yes.  However, I was
> > a bit worried that if sp804 is turned into a full driver, it will get
> > initialised twice -- once explicitly and once in of_platform_populate()...
> > at least until the baord code is adapted to work properly with the new
> > driver.
> > 
> > Commenting these entries out for now seemed a good idea to avoid the flag-day
> > hazard.  Am I being too cautious?
> 
> I think you are ;-) Besides my static-mapping-rework is already using
> those...

OK, I will uncomment it then.

Cheers
---Dave
Pawel Moll - Sept. 21, 2011, 4:01 p.m.
On Wed, 2011-09-21 at 15:57 +0100, Grant Likely wrote:
> On Wed, Sep 21, 2011 at 7:24 AM, Rob Herring <robherring2@gmail.com> wrote:
> > On 09/21/2011 04:19 AM, Dave Martin wrote:
> >>       * arm,amba-bus -- widely used by other boards and patchsets, but
> >>         seems not to be documented.
> >>
> >
> > This should be dropped. There's not really any bus component to an amba
> > bus. All the probing info is within the primecell peripherals.
> 
> No, if it is an AMBA bus, then it is entirely appropriate to declare
> it as an amba bus, but to also be compatible with "simple-bus".  In
> fact, it would be better to use a compatible string that specifies the
> specific implementation of AMBA bus since there are several versions
> of the spec.

Dave asked me about details of the VE implementation. It's
sort-of-complicated... ;-)

1. Core talks to Static Memory Controller via AMBA (AXI)

   SOC { core --AXI--> SMC }

2. SMC generates transaction on Static Memory Bus talking to the IO FPGA

   tile/motherboard connector { SMC --SMB--> IOFPGA }

3. Now, depending on the device being accessed:

a) Transactions accessing SMSC9118, ISP1761, NOR Flash and PSRAM are
routed directly to the devices

   IOFPGA { SMB --> SMSC9118 et al. }

b) The rest of the traffic is converted back to AMBA (AHB/APB)
transactions and sent to the devices connected to internal AMBA matrix.

   IOFPGA { SMB --> AHB/APB bus master --AHB/APB--> PL180 }

I don't believe, though, that the DTS must reflect such level of
details. That's why I think that:

+       motherboard {
+               compatible = "simple-bus";

and

+               peripherals@7,00000000 {
+                       compatible = "arm,amba-bus", "simple-bus";

is the best description of the reality :-)

Cheers!

Paweł
Dave Martin - Sept. 21, 2011, 4:17 p.m.
On Wed, Sep 21, 2011 at 5:01 PM, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2011-09-21 at 15:57 +0100, Grant Likely wrote:
>> On Wed, Sep 21, 2011 at 7:24 AM, Rob Herring <robherring2@gmail.com> wrote:
>> > On 09/21/2011 04:19 AM, Dave Martin wrote:
>> >>       * arm,amba-bus -- widely used by other boards and patchsets, but
>> >>         seems not to be documented.
>> >>
>> >
>> > This should be dropped. There's not really any bus component to an amba
>> > bus. All the probing info is within the primecell peripherals.
>>
>> No, if it is an AMBA bus, then it is entirely appropriate to declare
>> it as an amba bus, but to also be compatible with "simple-bus".  In
>> fact, it would be better to use a compatible string that specifies the
>> specific implementation of AMBA bus since there are several versions
>> of the spec.
>
> Dave asked me about details of the VE implementation. It's
> sort-of-complicated... ;-)
>
> 1. Core talks to Static Memory Controller via AMBA (AXI)
>
>   SOC { core --AXI--> SMC }
>
> 2. SMC generates transaction on Static Memory Bus talking to the IO FPGA
>
>   tile/motherboard connector { SMC --SMB--> IOFPGA }
>
> 3. Now, depending on the device being accessed:
>
> a) Transactions accessing SMSC9118, ISP1761, NOR Flash and PSRAM are
> routed directly to the devices
>
>   IOFPGA { SMB --> SMSC9118 et al. }
>
> b) The rest of the traffic is converted back to AMBA (AHB/APB)
> transactions and sent to the devices connected to internal AMBA matrix.
>
>   IOFPGA { SMB --> AHB/APB bus master --AHB/APB--> PL180 }
>
> I don't believe, though, that the DTS must reflect such level of
> details. That's why I think that:
>
> +       motherboard {
> +               compatible = "simple-bus";
>
> and
>
> +               peripherals@7,00000000 {
> +                       compatible = "arm,amba-bus", "simple-bus";
>
> is the best description of the reality :-)

I wonder whether an OS will ever need to know this detail.

Am I right in understanding that these buses are just interconnect
logic, with no OS-visible control/configuration interface?

Cheers
---Dave
Pawel Moll - Sept. 21, 2011, 4:28 p.m.
> > Dave asked me about details of the VE implementation. It's
> > sort-of-complicated... ;-)
> >
> > 1. Core talks to Static Memory Controller via AMBA (AXI)
> >
> >   SOC { core --AXI--> SMC }
> >
> > 2. SMC generates transaction on Static Memory Bus talking to the IO FPGA
> >
> >   tile/motherboard connector { SMC --SMB--> IOFPGA }
> >
> > 3. Now, depending on the device being accessed:
> >
> > a) Transactions accessing SMSC9118, ISP1761, NOR Flash and PSRAM are
> > routed directly to the devices
> >
> >   IOFPGA { SMB --> SMSC9118 et al. }
> >
> > b) The rest of the traffic is converted back to AMBA (AHB/APB)
> > transactions and sent to the devices connected to internal AMBA matrix.
> >
> >   IOFPGA { SMB --> AHB/APB bus master --AHB/APB--> PL180 }
> >
> > I don't believe, though, that the DTS must reflect such level of
> > details. That's why I think that:
> >
> > +       motherboard {
> > +               compatible = "simple-bus";
> >
> > and
> >
> > +               peripherals@7,00000000 {
> > +                       compatible = "arm,amba-bus", "simple-bus";
> >
> > is the best description of the reality :-)
> 
> I wonder whether an OS will ever need to know this detail.

Which one of the details you mean? Exact architecture describing what I
said above? I don't think so.

The compatible = "arm,amba-bus" for CS7? Probably not, but I think it's
good to have it there as it answers the question "so how can AMBA device
like PL180 be connected to a static memory bus?!?".

> Am I right in understanding that these buses are just interconnect
> logic, with no OS-visible control/configuration interface?

Definitely nothing publicly specified :-)

Cheers!

Paweł
Rob Herring - Sept. 21, 2011, 4:37 p.m.
On 09/21/2011 09:57 AM, Grant Likely wrote:
> On Wed, Sep 21, 2011 at 7:24 AM, Rob Herring <robherring2@gmail.com> wrote:
>> On 09/21/2011 04:19 AM, Dave Martin wrote:
>>>       * arm,amba-bus -- widely used by other boards and patchsets, but
>>>         seems not to be documented.
>>>
>>
>> This should be dropped. There's not really any bus component to an amba
>> bus. All the probing info is within the primecell peripherals.
> 
> No, if it is an AMBA bus, then it is entirely appropriate to declare
> it as an amba bus, but to also be compatible with "simple-bus".  In
> fact, it would be better to use a compatible string that specifies the
> specific implementation of AMBA bus since there are several versions
> of the spec.

And type of AMBA bus as the spec includes AXI, AHB, and APB. None of
which have any sort of programmability or software view.

If this is required, then the policy should be simple-bus should never
be allowed alone as every bus has some underlying type. Seems like
overkill for buses like this.

Rob
Dave Martin - Sept. 21, 2011, 5:15 p.m.
On Wed, Sep 21, 2011 at 11:37:54AM -0500, Rob Herring wrote:
> On 09/21/2011 09:57 AM, Grant Likely wrote:
> > On Wed, Sep 21, 2011 at 7:24 AM, Rob Herring <robherring2@gmail.com> wrote:
> >> On 09/21/2011 04:19 AM, Dave Martin wrote:
> >>>       * arm,amba-bus -- widely used by other boards and patchsets, but
> >>>         seems not to be documented.
> >>>
> >>
> >> This should be dropped. There's not really any bus component to an amba
> >> bus. All the probing info is within the primecell peripherals.
> > 
> > No, if it is an AMBA bus, then it is entirely appropriate to declare
> > it as an amba bus, but to also be compatible with "simple-bus".  In
> > fact, it would be better to use a compatible string that specifies the
> > specific implementation of AMBA bus since there are several versions
> > of the spec.
> 
> And type of AMBA bus as the spec includes AXI, AHB, and APB. None of
> which have any sort of programmability or software view.
> 
> If this is required, then the policy should be simple-bus should never
> be allowed alone as every bus has some underlying type. Seems like
> overkill for buses like this.

The key question is _where_ to draw the line between generic and specific.
By definition, the DT can never be a comprehensive description of the
hardware -- rather a good DT is a description of those details of the hardware
which could relevant to any hypothetical OS.

The flipside is that details which were thought to be irrelevant at
design/implementation time can turn out to be relevant in practice, due
to errata and implementation issues etc.  So taking the description slightly
beyond what the OS needs to know can still have some merit.


I still don't know how to say where the line should be drawn in this particular
case though.

Cheers
---Dave
Mitch Bradley - Sept. 21, 2011, 5:47 p.m.
On 9/21/2011 7:15 AM, Dave Martin wrote:
> On Wed, Sep 21, 2011 at 11:37:54AM -0500, Rob Herring wrote:
>> On 09/21/2011 09:57 AM, Grant Likely wrote:
>>> On Wed, Sep 21, 2011 at 7:24 AM, Rob Herring<robherring2@gmail.com>  wrote:
>>>> On 09/21/2011 04:19 AM, Dave Martin wrote:
>>>>>        * arm,amba-bus -- widely used by other boards and patchsets, but
>>>>>          seems not to be documented.
>>>>>
>>>>
>>>> This should be dropped. There's not really any bus component to an amba
>>>> bus. All the probing info is within the primecell peripherals.
>>>
>>> No, if it is an AMBA bus, then it is entirely appropriate to declare
>>> it as an amba bus, but to also be compatible with "simple-bus".  In
>>> fact, it would be better to use a compatible string that specifies the
>>> specific implementation of AMBA bus since there are several versions
>>> of the spec.
>>
>> And type of AMBA bus as the spec includes AXI, AHB, and APB. None of
>> which have any sort of programmability or software view.
>>
>> If this is required, then the policy should be simple-bus should never
>> be allowed alone as every bus has some underlying type. Seems like
>> overkill for buses like this.
>
> The key question is _where_ to draw the line between generic and specific.
> By definition, the DT can never be a comprehensive description of the
> hardware -- rather a good DT is a description of those details of the hardware
> which could relevant to any hypothetical OS.
>
> The flipside is that details which were thought to be irrelevant at
> design/implementation time can turn out to be relevant in practice, due
> to errata and implementation issues etc.  So taking the description slightly
> beyond what the OS needs to know can still have some merit.
>
>
> I still don't know how to say where the line should be drawn in this particular
> case though.

Here are some criteria:

If the controller for the bus itself has registers, include the bus node

If it is possible to plug new stuff into the bus, include the bus node

If the base address for the bus can be changed, thereby changing all the 
addresses of its subordinates by the same offset, include the bus node 
(this usually goes along with "has registers".)

ARM buses typically don't have any of those attributes, but there are 
some weaker criteria that can be used to justify including a bus node. 
The SoC on which I'm currently working has some peripherals on AXI and 
others on APB.  That doesn't matter from that addressing standpoint - 
the individual peripherals can each be viewed as having an address, end 
of story - but it does matter from a power management standpoint.  The 
clock tree is quite related to the bus layout.  Including bus nodes in 
the device tree might provide useful place-holders for properties 
describing power or clock domains.

So, on the whole, I'm in favor of including bus nodes for ARM standard 
buses.  There is little down side to doing so, and a fair chance that it 
might come in handy in the future.

>
> Cheers
> ---Dave
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
Dave Martin - Sept. 22, 2011, 12:19 p.m.
On Wed, Sep 21, 2011 at 07:47:32AM -1000, Mitch Bradley wrote:
> On 9/21/2011 7:15 AM, Dave Martin wrote:
> >On Wed, Sep 21, 2011 at 11:37:54AM -0500, Rob Herring wrote:
> >>On 09/21/2011 09:57 AM, Grant Likely wrote:
> >>>On Wed, Sep 21, 2011 at 7:24 AM, Rob Herring<robherring2@gmail.com>  wrote:
> >>>>On 09/21/2011 04:19 AM, Dave Martin wrote:
> >>>>>       * arm,amba-bus -- widely used by other boards and patchsets, but
> >>>>>         seems not to be documented.
> >>>>>
> >>>>
> >>>>This should be dropped. There's not really any bus component to an amba
> >>>>bus. All the probing info is within the primecell peripherals.
> >>>
> >>>No, if it is an AMBA bus, then it is entirely appropriate to declare
> >>>it as an amba bus, but to also be compatible with "simple-bus".  In
> >>>fact, it would be better to use a compatible string that specifies the
> >>>specific implementation of AMBA bus since there are several versions
> >>>of the spec.
> >>
> >>And type of AMBA bus as the spec includes AXI, AHB, and APB. None of
> >>which have any sort of programmability or software view.
> >>
> >>If this is required, then the policy should be simple-bus should never
> >>be allowed alone as every bus has some underlying type. Seems like
> >>overkill for buses like this.
> >
> >The key question is _where_ to draw the line between generic and specific.
> >By definition, the DT can never be a comprehensive description of the
> >hardware -- rather a good DT is a description of those details of the hardware
> >which could relevant to any hypothetical OS.
> >
> >The flipside is that details which were thought to be irrelevant at
> >design/implementation time can turn out to be relevant in practice, due
> >to errata and implementation issues etc.  So taking the description slightly
> >beyond what the OS needs to know can still have some merit.
> >
> >
> >I still don't know how to say where the line should be drawn in this particular
> >case though.
> 
> Here are some criteria:
> 
> If the controller for the bus itself has registers, include the bus node
> 
> If it is possible to plug new stuff into the bus, include the bus node
> 
> If the base address for the bus can be changed, thereby changing all
> the addresses of its subordinates by the same offset, include the
> bus node (this usually goes along with "has registers".)
> 
> ARM buses typically don't have any of those attributes, but there
> are some weaker criteria that can be used to justify including a bus
> node. The SoC on which I'm currently working has some peripherals on
> AXI and others on APB.  That doesn't matter from that addressing
> standpoint - the individual peripherals can each be viewed as having
> an address, end of story - but it does matter from a power
> management standpoint.  The clock tree is quite related to the bus
> layout.  Including bus nodes in the device tree might provide useful
> place-holders for properties describing power or clock domains.
> 
> So, on the whole, I'm in favor of including bus nodes for ARM
> standard buses.  There is little down side to doing so, and a fair
> chance that it might come in handy in the future.

I'm guessing that that would motivate describing most of the bus layout,
with the possible exception of bus adaptors which exist solely for the
purpose of integrating a single slave with the parent bus -- providing
such adaptors are transparent to software.

I'll have a think about what we would need to add...

Cheers
---Dave
Tabi Timur-B04825 - Jan. 9, 2012, 11:26 p.m.
On Wed, Sep 21, 2011 at 4:19 AM, Dave Martin <dave.martin@linaro.org> wrote:
>
>      * edid -- It should be possible to have a fairly generic binding
>        for EDID interfaces, but none seems to exist yet.  Discussion
>        is needed regarding what form this should take.
>
>        This might more appropriately be called "ddc" (or some
>        variation on that), since EDID seems only to describe the
>        format of the ID data retrievable via this interface; not the
>        interface itself.

Has there been any progress on this issue?  I'm looking to add EDID
support to a PowerPC device tree.  A TI developer is using
"ti,eeprom", but I'm not sure that's a good choice.
Mitch Bradley - Jan. 10, 2012, 12:42 a.m.
On 1/9/2012 1:26 PM, Tabi Timur-B04825 wrote:
> On Wed, Sep 21, 2011 at 4:19 AM, Dave Martin<dave.martin@linaro.org>  wrote:
>>
>>       * edid -- It should be possible to have a fairly generic binding
>>         for EDID interfaces, but none seems to exist yet.  Discussion
>>         is needed regarding what form this should take.
>>
>>         This might more appropriately be called "ddc" (or some
>>         variation on that), since EDID seems only to describe the
>>         format of the ID data retrievable via this interface; not the
>>         interface itself.
>
> Has there been any progress on this issue?  I'm looking to add EDID
> support to a PowerPC device tree.  A TI developer is using
> "ti,eeprom", but I'm not sure that's a good choice.
>

The way it works for many "graphics cards" is that the display hardware 
subsystem includes an I2C (also called "SMBUS") interface that connects 
to the EDID ROM on the monitor.  In this model, the EDID interface is 
not a standalone device, but rather a feature of the display device.

In that scenario, the EDID-reading code is just part of the display 
driver, so you don't need a separate device node.

If the display hardware does not include a dedicated I2C interface 
intended for EDID, then I think what you need is a way to associate an 
external I2C interface with the display driver for that hardware.  The 
interpretation of the data as EDID is not really part of the hardware 
interface, but rather a function of the display driver.  Therefore, I 
think the right way to look at this is not to have a binding for "EDID 
interfaces", but rather a convention for associating a specific instance 
of an I2C interface with a display driver.

The obvious way to do that would be to have a property in the display 
driver whose value is the phandle of an i2c device node.  The display 
driver can then use that to read and interpret the EDID bytes.

In my opinion, pushing the EDID abstraction into a node by itself is not 
worthwhile.  The EDID spec says that you read either 128 or 256 bytes 
from an I2C device at I2C address 0x50; you hardly need an abstraction 
for that, given that you have a "read from I2C" method.
The right level of abstraction at the device node level is "this 
hardware implements an I2C bus master", for which there is already a 
binding.  Then all you need is a reference to that device from a display 
device node.

The display device driver will need to interpret the EDID data in an 
device-dependent manner.  That is inherent in the fact that the driver 
for the given display hardware must map the EDID description of the 
monitor into display-hardware-dependent timing settings.

Some I2C interfaces are implemented by bit-banging GPIOs, while others 
use dedicated hardware protocol engines.  The display driver need not 
know or care about that, as it should be hidden by the i2c bus abstraction.
Tabi Timur-B04825 - Jan. 10, 2012, 2:24 a.m.
Mitch Bradley wrote:

> The way it works for many "graphics cards" is that the display hardware
> subsystem includes an I2C (also called "SMBUS") interface that connects to
> the EDID ROM on the monitor. In this model, the EDID interface is not a
> standalone device, but rather a feature of the display device.
>
> In that scenario, the EDID-reading code is just part of the display
> driver, so you don't need a separate device node.

On the system I'm supporting, the I2C bus is not part of the video 
hardware.  The EDID "device" is not even on a dedicated bus -- it's on a 
shared I2C bus with other devices.

> If the display hardware does not include a dedicated I2C interface
> intended for EDID, then I think what you need is a way to associate an
> external I2C interface with the display driver for that hardware. The
> interpretation of the data as EDID is not really part of the hardware
> interface, but rather a function of the display driver. Therefore, I think
> the right way to look at this is not to have a binding for "EDID
> interfaces", but rather a convention for associating a specific instance
> of an I2C interface with a display driver.

I don't think that's going to work for me.  Reading the EDID data is a 
platform function, not a video driver function.  I'm adding platform code 
to read the EDID data.

We can't really make it generic, either, even though it's using address 
0x50 like everyone else.  On my platform, for instance, I need to enable 
the EDID interface via an FPGA, perform the I2C read, and then disable the 
EDID interface.

> The obvious way to do that would be to have a property in the display
> driver whose value is the phandle of an i2c device node. The display
> driver can then use that to read and interpret the EDID bytes.

Hmmm.... now that I think about it, I can create platform-specific "EDID 
enable" and "EDID disable" functions, and let the video driver do the I2C 
load generically, using a phandle.

> In my opinion, pushing the EDID abstraction into a node by itself is not
> worthwhile.

Well, I have to create an I2C device node in order to get any I2C working.

> The EDID spec says that you read either 128 or 256 bytes from
> an I2C device at I2C address 0x50; you hardly need an abstraction for
> that, given that you have a "read from I2C" method.
> The right level of abstraction at the device node level is "this hardware
> implements an I2C bus master", for which there is already a binding. Then
> all you need is a reference to that device from a display device node.
>
> The display device driver will need to interpret the EDID data in an
> device-dependent manner. That is inherent in the fact that the driver for
> the given display hardware must map the EDID description of the monitor
> into display-hardware-dependent timing settings.
>
> Some I2C interfaces are implemented by bit-banging GPIOs, while others use
> dedicated hardware protocol engines. The display driver need not know or
> care about that, as it should be hidden by the i2c bus abstraction.

This gives me something to think about.  Thanks.
Dave Martin - Jan. 10, 2012, 11:04 a.m.
On Mon, Jan 09, 2012 at 11:26:38PM +0000, Tabi Timur-B04825 wrote:
> On Wed, Sep 21, 2011 at 4:19 AM, Dave Martin <dave.martin@linaro.org> wrote:
> >
> > ? ? ?* edid -- It should be possible to have a fairly generic binding
> > ? ? ? ?for EDID interfaces, but none seems to exist yet. ?Discussion
> > ? ? ? ?is needed regarding what form this should take.
> >
> > ? ? ? ?This might more appropriately be called "ddc" (or some
> > ? ? ? ?variation on that), since EDID seems only to describe the
> > ? ? ? ?format of the ID data retrievable via this interface; not the
> > ? ? ? ?interface itself.
> 
> Has there been any progress on this issue?  I'm looking to add EDID
> support to a PowerPC device tree.  A TI developer is using
> "ti,eeprom", but I'm not sure that's a good choice.

It turns out that because of the way things are wired up on vexpress, the
EDID is not really usable; so I wasn't planning to do anything about it.


I don't really know enough about this field to comment on whether it's
genuinely useful to have a specific binding for EDID.  If it's enough
to reference the appropriate I2C bus node from the display device node,
then I guess we don't necessarily need to worry about having a separate
binding.

If it makes no sense to attempt make EDID access fully generic, that
would sound like the right approach.

Cheers
---Dave
Jamie Lokier - Jan. 10, 2012, 12:22 p.m.
Mitch Bradley wrote:
> In my opinion, pushing the EDID abstraction into a node by itself is
> not worthwhile.  The EDID spec says that you read either 128 or 256
> bytes from an I2C device at I2C address 0x50; you hardly need an
> abstraction for that, given that you have a "read from I2C" method.

Since E-EDID and E-DDC, it's up to 32k in 256 byte pages, and you need
a special way of reading beyond the first 128 bytes, by writing page
register at 0x30; and it must be all in a single I2C transaction as
the page register is reset by STOP events.

Also on some monitors there's DDC-CI protocol on a different I2C
address for controlling the monitor. It's not very useful in practice
IME, but some may want to use it, for which userspace needs DDC access.

If you mimic what X.org and some kernel graphics drivers do with DDC,
there's some extra signal wiggling that ought to happen beyond I2C
protocol for compatibility with older monitors.  But that's not
mentioned in the DDC specs that I've seen.

Unlike I2C itself, DDC has timeouts if there's no response, in which
case an I2C reset sequence is a good idea after.  It makes sense for
the timeouts and resets to be the same for all display drivers.

Reading over DDC can become a random write if there's a signal glitch,
like bounce when someone removes a cable.  Unfortunately some monitors
store the written data permanently, wiping their EDID or part of it.
(I suspect this is actually the reason the kernel EDID code has (or
had?) a workaround to ignore the first four bytes for the checksum.)
This can happen especially if EDID is polled in the background to
check for cable removal.  It's possible to guard against this (or
limit the damage) by checking the display doesn't assert I2C data for
more than 8 cycles in a row.

The EDID data of the currently connected display, and notification
when it changes or is detached, are quite useful to userspace in my
experience.  Preferably cached, periodically validated.

> The right level of abstraction at the device node level is "this
> hardware implements an I2C bus master", for which there is already a
> binding.  Then all you need is a reference to that device from a
> display device node.

It still needs to know *which* I2C bus master is connected to the
display. Some graphics hardware has multiple, e.g. one to talk with
the DVI/HDMI transmitter, another connected by cable to the display.

Since the I2C connected to the display is officially called DDC
(actually E-DDC now) that seems a natural name for the node.

I have worked with devices that shared the same I2C for DDC and
talking with on-board devices. (That was a bad idea, as some monitors
clamp the lines high or low excessively even when switched off.) Point
here is sometimes there's a dedicated one for DDC; sometimes there isn't.

> The display device driver will need to interpret the EDID data in an
> device-dependent manner.  That is inherent in the fact that the
> driver for the given display hardware must map the EDID description
> of the monitor into display-hardware-dependent timing settings.

For the most part, EDID's timings are independent of the display
driver except the chosen timings will have to satisfy both sets of
constraints (sender and receiver). Decoding E-EDID fully is
surprisingly big and complicated, not to mention the workarounds for
dodgy EDIDs (see X.org source) so it's really best to not duplicate it.

All the best,
-- Jamie
Timur Tabi - Jan. 10, 2012, 9:58 p.m.
Jamie Lokier wrote:

> It still needs to know *which* I2C bus master is connected to the
> display. Some graphics hardware has multiple, e.g. one to talk with
> the DVI/HDMI transmitter, another connected by cable to the display.

Yes, this is my problem.  I have multiple I2C busses on my system, and there's no way to guess which one is connected to the DVI port.  A phandle in the device tree is the only way I can figure out which I2C bus to use.  But there's another problem.  I can use of_find_i2c_device_by_node() to determine the i2c_client (and therefore, the i2c_adapter) to use for fb_ddc_read().  However, that function only works if the I2C device was probed.  That typically is done by the I2C driver for the device, but there is no "edid" device driver.  So my framebuffer driver is going to have to pretend to be one.

I wish there were some way to obtain the i2c_adapter struct more easily.

> I have worked with devices that shared the same I2C for DDC and
> talking with on-board devices. (That was a bad idea, as some monitors
> clamp the lines high or low excessively even when switched off.) Point
> here is sometimes there's a dedicated one for DDC; sometimes there isn't.

I also have this problem.  I need to toggle a GPIO in order to get the I2C bus connected to the DVI port.  The same I2C bus is used for the audio codec and a bunch of other devices.  Our SOC has four I2C busses, so I don't understand why the board designed didn't dedicate one for the DDC.  But I can easily work around this issue.
Mitch Bradley - Jan. 10, 2012, 10:35 p.m.
On 1/10/2012 11:58 AM, Timur Tabi wrote:
> Jamie Lokier wrote:
>
>> It still needs to know *which* I2C bus master is connected to the
>> display. Some graphics hardware has multiple, e.g. one to talk with
>> the DVI/HDMI transmitter, another connected by cable to the display.
>
> Yes, this is my problem.  I have multiple I2C busses on my system, and there's no way to guess which one is connected to the DVI port.  A phandle in the device tree is the only way I can figure out which I2C bus to use.  But there's another problem.  I can use of_find_i2c_device_by_node() to determine the i2c_client (and therefore, the i2c_adapter) to use for fb_ddc_read().  However, that function only works if the I2C device was probed.  That typically is done by the I2C driver for the device, but there is no "edid" device driver.  So my framebuffer driver is going to have to pretend to be one.

It seems to me that having the framebuffer driver handle the EDID 
"driver" function is exactly the right thing.  The framebuffer driver is 
the only thing that cares about EDID information.  Given a phandle 
reference to an I2C interface, the "driver" is just "call the I2C read 
routine" - plus, in your case, toggling the GPIO pin.

That GPIO pin thing is annoying, but not sufficiently complex or common 
that it warrants having a separate EDID driver.  You could define a 
platform-specific property to tell your framebuffer driver that it needs 
to do that GPIO thing.  It's a hack, but the GPIO thing is inherently a 
hack, so there will be some ugliness somewhere as a result.

>
> I wish there were some way to obtain the i2c_adapter struct more easily.
>
>> I have worked with devices that shared the same I2C for DDC and
>> talking with on-board devices. (That was a bad idea, as some monitors
>> clamp the lines high or low excessively even when switched off.) Point
>> here is sometimes there's a dedicated one for DDC; sometimes there isn't.
>
> I also have this problem.  I need to toggle a GPIO in order to get the I2C bus connected to the DVI port.  The same I2C bus is used for the audio codec and a bunch of other devices.  Our SOC has four I2C busses, so I don't understand why the board designed didn't dedicate one for the DDC.  But I can easily work around this issue.
>
>
Stephen Warren - Jan. 10, 2012, 11:55 p.m.
Mitch Bradley wrote at Tuesday, January 10, 2012 3:36 PM:
> On 1/10/2012 11:58 AM, Timur Tabi wrote:
> > Jamie Lokier wrote:
> >
> >> It still needs to know *which* I2C bus master is connected to the
> >> display. Some graphics hardware has multiple, e.g. one to talk with
> >> the DVI/HDMI transmitter, another connected by cable to the display.
> >
> > Yes, this is my problem.  I have multiple I2C busses on my system, and there's no way to guess which
> one is connected to the DVI port.  A phandle in the device tree is the only way I can figure out which
> I2C bus to use.  But there's another problem.  I can use of_find_i2c_device_by_node() to determine the
> i2c_client (and therefore, the i2c_adapter) to use for fb_ddc_read().  However, that function only
> works if the I2C device was probed.  That typically is done by the I2C driver for the device, but
> there is no "edid" device driver.  So my framebuffer driver is going to have to pretend to be one.
> 
> It seems to me that having the framebuffer driver handle the EDID
> "driver" function is exactly the right thing.  The framebuffer driver is
> the only thing that cares about EDID information.  Given a phandle
> reference to an I2C interface, the "driver" is just "call the I2C read
> routine" - plus, in your case, toggling the GPIO pin.
> 
> That GPIO pin thing is annoying, but not sufficiently complex or common
> that it warrants having a separate EDID driver.  You could define a
> platform-specific property to tell your framebuffer driver that it needs
> to do that GPIO thing.  It's a hack, but the GPIO thing is inherently a
> hack, so there will be some ugliness somewhere as a result.

Shouldn't there be an I2C bus mux driver to handle this? The kernel
already has support for bus muxes controller over I2C. I think here you
just need to implement the same thing, but with the control via a GPIO.
In the future I hope to implement something similar but controller using
an SoC's pinmux HW (where a single controller's I2C signals can be routed
to different sets of pins on the SoC using its pinmux) - a feature which
at least some Tegra boards use for sharing DDC and something else. Such
a mux can be explicitly represented in DT, with multiple I2C child bus
nodes, I think.

> > I wish there were some way to obtain the i2c_adapter struct more easily.
> >
> >> I have worked with devices that shared the same I2C for DDC and
> >> talking with on-board devices. (That was a bad idea, as some monitors
> >> clamp the lines high or low excessively even when switched off.) Point
> >> here is sometimes there's a dedicated one for DDC; sometimes there isn't.
> >
> > I also have this problem.  I need to toggle a GPIO in order to get the I2C bus connected to the DVI
> port.  The same I2C bus is used for the audio codec and a bunch of other devices.  Our SOC has four
> I2C busses, so I don't understand why the board designed didn't dedicate one for the DDC.  But I can
> easily work around this issue.
Timur Tabi - Jan. 11, 2012, 12:02 a.m.
Stephen Warren wrote:
> Shouldn't there be an I2C bus mux driver to handle this? The kernel
> already has support for bus muxes controller over I2C. 

Ah, I did not know that.  I'll look into it.  Thanks.
Timur Tabi - Jan. 11, 2012, 12:28 a.m.
Mitch Bradley wrote:
> It seems to me that having the framebuffer driver handle the EDID 
> "driver" function is exactly the right thing.  The framebuffer driver is 
> the only thing that cares about EDID information.  Given a phandle 
> reference to an I2C interface, the "driver" is just "call the I2C read 
> routine" - plus, in your case, toggling the GPIO pin.

Do you think I should have the I2C probe function read the EDID data, and then store it in some global variable?  That won't work if I have multiple video controllers, since I won't know which EDID data goes with which video controller.  I tried adding a call to i2c_add_driver() in my framebuffer driver, but the I2C probe function was called *after* the framebuffer's probe function, so that doesn't help me.  

> That GPIO pin thing is annoying, but not sufficiently complex or common 
> that it warrants having a separate EDID driver.  You could define a 
> platform-specific property to tell your framebuffer driver that it needs 
> to do that GPIO thing.  It's a hack, but the GPIO thing is inherently a 
> hack, so there will be some ugliness somewhere as a result.

I have two platform-specific functions, "enabled_edid" and "disable_edid", that I call before/after calling fb_ddc_read().  This seems to work well, and I already have a mechanism for calling platform-specific functions from the framebuffer driver.

However, Stephen Warren said I should be using the I2C mux feature instead.
Mitch Bradley - Jan. 11, 2012, 6:43 a.m.
On 1/10/2012 2:28 PM, Timur Tabi wrote:
> Mitch Bradley wrote:
>> It seems to me that having the framebuffer driver handle the EDID
>> "driver" function is exactly the right thing.  The framebuffer driver is
>> the only thing that cares about EDID information.  Given a phandle
>> reference to an I2C interface, the "driver" is just "call the I2C read
>> routine" - plus, in your case, toggling the GPIO pin.
>
> Do you think I should have the I2C probe function read the EDID data, and then store it in some global variable?

I think that would not be a good design.  Presence and location of EDID 
data is not something that a generic I2C driver should know.  It's the 
video controller that knows that EDID exists, where it is located 
(device ID 0x50 and addresses within that device), and what it means.

>  That won't work if I have multiple video controllers, since I won't know which EDID data goes with which video controller.  I tried adding a call to i2c_add_driver() in my framebuffer driver, but the I2C probe function was called *after* the framebuffer's probe function, so that doesn't help me.

Then there needs to be some sort of ordering or dependency to ensure 
that the I2C driver is activated before the framebuffer driver tries to 
use it.

>
>> That GPIO pin thing is annoying, but not sufficiently complex or common
>> that it warrants having a separate EDID driver.  You could define a
>> platform-specific property to tell your framebuffer driver that it needs
>> to do that GPIO thing.  It's a hack, but the GPIO thing is inherently a
>> hack, so there will be some ugliness somewhere as a result.
>
> I have two platform-specific functions, "enabled_edid" and "disable_edid", that I call before/after calling fb_ddc_read().  This seems to work well, and I already have a mechanism for calling platform-specific functions from the framebuffer driver.
>
> However, Stephen Warren said I should be using the I2C mux feature instead.
>

I2C mux is a plausible solution, as is your enable/disable thing.  At 
some level they are equivalent.  I2C mux is a formalization of your 
solution, in which the mux device's select method must be written to 
perform the function of your enable/disable edid functions.

Either way, you need platform-dependent functions to do the switching, 
and you need to select the appropriate channel.  Personally, I don't see 
the advantage of using the mux device in this case.  It's just adding 
complexity with no payback.  If there were several channels that needed 
to be accessed in an interspersed fashion, the mux device would  be much 
cleaner.  But in this case, there is a single "back channel" that only 
needs to be accessed once and can subsequently be ignored.  The video 
driver can grab a lock, call enable_edid(), read out the EDID data into 
an array, call disable_edid(), release the lock, and that is it.  The 
other users of that I2C bus can ignore the hidden EDID.

But, as I said, either way is okay.  They are pretty much equivalent at 
a fundamental level.  You must have the platform-dependent function to 
do the switching, and you must have a reference to an underlying I2C 
bus.  If you go the mux route, every other user of that I2C bus must 
also use the mux device, selecting the other channel.  That propagates 
the knowledge of this "hidden" EDID ROM farther than it would otherwise 
have to go.  It probably also means you would want a device node to 
represent the fact that the I2C bus is muxed, to instantiate the i2c_mux 
driver.  I think that doing a binding for i2c_mux is a can of worms, 
because of the need to represent the platform-dependent "how to select" 
functionality.
Timur Tabi - Jan. 11, 2012, 8:17 p.m.
Mitch Bradley wrote:

> I think that would not be a good design.  Presence and location of EDID 
> data is not something that a generic I2C driver should know.  It's the 
> video controller that knows that EDID exists, where it is located 
> (device ID 0x50 and addresses within that device), and what it means.

But the video driver does not know what I2C *bus* it's on.  I have been unable to come up with a way to obtain the proper i2c_adapter object to use when looking for EDID data.  

>>  That won't work if I have multiple video controllers, since I won't know which EDID data goes with which video controller.  I tried adding a call to i2c_add_driver() in my framebuffer driver, but the I2C probe function was called *after* the framebuffer's probe function, so that doesn't help me.
> 
> Then there needs to be some sort of ordering or dependency to ensure 
> that the I2C driver is activated before the framebuffer driver tries to 
> use it.

I don't know how to do that, either.

> Either way, you need platform-dependent functions to do the switching, 
> and you need to select the appropriate channel.  Personally, I don't see 
> the advantage of using the mux device in this case.  It's just adding 
> complexity with no payback.

I'm always in favor of doing things the simpler way.
Stephen Warren - Jan. 11, 2012, 8:29 p.m.
Mitch Bradley wrote at Tuesday, January 10, 2012 11:43 PM:
> On 1/10/2012 2:28 PM, Timur Tabi wrote:
> > Mitch Bradley wrote:
...
> >> That GPIO pin thing is annoying, but not sufficiently complex or common
> >> that it warrants having a separate EDID driver.  You could define a
> >> platform-specific property to tell your framebuffer driver that it needs
> >> to do that GPIO thing.  It's a hack, but the GPIO thing is inherently a
> >> hack, so there will be some ugliness somewhere as a result.
> >
> > I have two platform-specific functions, "enabled_edid" and "disable_edid", that I call before/after
> > calling fb_ddc_read().  This seems to work well, and I already have a mechanism for calling platform-
> > specific functions from the framebuffer driver.
> >
> > However, Stephen Warren said I should be using the I2C mux feature instead.
> 
> I2C mux is a plausible solution, as is your enable/disable thing.  At
> some level they are equivalent.  I2C mux is a formalization of your
> solution, in which the mux device's select method must be written to
> perform the function of your enable/disable edid functions.
> 
> Either way, you need platform-dependent functions to do the switching,
> and you need to select the appropriate channel.  Personally, I don't see
> the advantage of using the mux device in this case.

The main advantage I see is that you explicitly don't need any platform-
specific functions to do the switching; you end up with platform-agnostic
code (the I2C GPIO mux driver) and platform-specific configuration for
that driver (the GPIO ID to use). The display driver just talks to the
I2C API for the DDC I2C bus, and doesn't do anything to switch between
the busses; the I2C GPIO mux driver does all that internally. Thus, the
display driver will work fine on boards that don't need this muxing with
zero changes; the board simply wouldn't register the mux driver.

> It's just adding
> complexity with no payback.  If there were several channels that needed
> to be accessed in an interspersed fashion, the mux device would  be much
> cleaner.  But in this case, there is a single "back channel" that only
> needs to be accessed once and can subsequently be ignored.

Well, the EDID needs to be read on every hotplug event, so it's certainly
not a one-time thing.

> The video
> driver can grab a lock, call enable_edid(), read out the EDID data into
> an array, call disable_edid(), release the lock, and that is it.  The
> other users of that I2C bus can ignore the hidden EDID.

Other I2C users/devices also shouldn't be impacted by the mux; they
would continue to use the existing I2C APIs for the bus their devices
are attached to, and not know about the mux. The overhead should be
almost zero; the I2C GPIO mux driver could remember the previous bus
selection and only modify it (gpio_set_value) when switching between
busses. I guess there might be a small overhead to taking a lock to
protect the bus selection; I'm not sure whether the I2C core serializes
access already or you'd need to add this; check the existing I2C I2C
bus mux implementation I guess.
Timur Tabi - Jan. 11, 2012, 8:32 p.m.
Stephen Warren wrote:
> Well, the EDID needs to be read on every hotplug event, so it's certainly
> not a one-time thing.

What is the hotplug event that triggers an EDID read?
Stephen Warren - Jan. 11, 2012, 8:36 p.m.
Timur Tabi wrote at Wednesday, January 11, 2012 1:33 PM:
> Stephen Warren wrote:
> > Well, the EDID needs to be read on every hotplug event, so it's certainly
> > not a one-time thing.
> 
> What is the hotplug event that triggers an EDID read?

For monitors attached to user-accessible connectors, whenever the user
physically plugs in a new monitor, you need to read the EDID since it
could be a new monitor. DVI and HDMI have explicit hotplug detect lines
to trigger this. I'm not sure how it works for VGA; perhaps you have to
manually reprobe.

Of course, if this thread is talking about built-in LCD displays where
the connectivity is fixed, this is a non-issue, but it's still a relevant
general design discussion; hopefully whatever solution that's picked would
work for DVI/HDMI even if you're dealing with LCDs right now.
Timur Tabi - Jan. 11, 2012, 9:37 p.m.
Stephen Warren wrote:
> For monitors attached to user-accessible connectors, whenever the 
> user physically plugs in a new monitor, you need to read the EDID 
> since it could be a new monitor. DVI and HDMI have explicit hotplug 
> detect lines to trigger this. I'm not sure how it works for VGA; 
> perhaps you have to manually reprobe.

We're using DVI, but it turns out that the interrupt signal on the video encoder (TFP410) is not connected to anything on our board, so I can't receive any interrupts.  The only way I would be able to tell is to create a kernel timer to poll the TFP410 through I2C, which I really don't want to do.

> Of course, if this thread is talking about built-in LCD displays 
> where the connectivity is fixed, this is a non-issue, but it's still 
> a relevant general design discussion; hopefully whatever solution 
> that's picked would work for DVI/HDMI even if you're dealing with 
> LCDs right now.

My board supports both LVDS and DVI, and the LVDS is connected to a built-in display, so I need to support both hotplug and non-hotplug.

I also noticed that the I2C interface is not enabled until after the console opens the framebuffer.  That means it's impossible for EDID data to be available before the console switches to the video device.  That doesn't seem right.
Stephen Warren - Jan. 11, 2012, 9:57 p.m.
Timur Tabi wrote at Wednesday, January 11, 2012 2:37 PM:
...
> I also noticed that the I2C interface is not enabled until after the console opens the framebuffer.
> That means it's impossible for EDID data to be available before the console switches to the video
> device.  That doesn't seem right.

I think what you need is this:

https://lkml.org/lkml/2011/10/7/14

That may not be the latest version of the patches; I'm not quite sure
what the status is right now.

Then, FB probe could be deferred until after I2C probe, and the EDID
would be available.
Mitch Bradley - Jan. 11, 2012, 11:16 p.m.
On 1/11/2012 10:29 AM, Stephen Warren wrote:
> Mitch Bradley wrote at Tuesday, January 10, 2012 11:43 PM:
>> On 1/10/2012 2:28 PM, Timur Tabi wrote:
>>> Mitch Bradley wrote:
> ...
>>>> That GPIO pin thing is annoying, but not sufficiently complex or common
>>>> that it warrants having a separate EDID driver.  You could define a
>>>> platform-specific property to tell your framebuffer driver that it needs
>>>> to do that GPIO thing.  It's a hack, but the GPIO thing is inherently a
>>>> hack, so there will be some ugliness somewhere as a result.
>>>
>>> I have two platform-specific functions, "enabled_edid" and "disable_edid", that I call before/after
>>> calling fb_ddc_read().  This seems to work well, and I already have a mechanism for calling platform-
>>> specific functions from the framebuffer driver.
>>>
>>> However, Stephen Warren said I should be using the I2C mux feature instead.
>>
>> I2C mux is a plausible solution, as is your enable/disable thing.  At
>> some level they are equivalent.  I2C mux is a formalization of your
>> solution, in which the mux device's select method must be written to
>> perform the function of your enable/disable edid functions.
>>
>> Either way, you need platform-dependent functions to do the switching,
>> and you need to select the appropriate channel.  Personally, I don't see
>> the advantage of using the mux device in this case.
>
> The main advantage I see is that you explicitly don't need any platform-
> specific functions to do the switching; you end up with platform-agnostic
> code (the I2C GPIO mux driver) and platform-specific configuration for
> that driver (the GPIO ID to use).


Oh, I didn't know about the I2C GPIO Mux driver.  I was looking at 
i2c-mux.c .  I now see gpio-i2cmux.c, which indeed seems to do the right 
thing.

> The display driver just talks to the
> I2C API for the DDC I2C bus, and doesn't do anything to switch between
> the busses; the I2C GPIO mux driver does all that internally. Thus, the
> display driver will work fine on boards that don't need this muxing with
> zero changes; the board simply wouldn't register the mux driver.
>
>> It's just adding
>> complexity with no payback.  If there were several channels that needed
>> to be accessed in an interspersed fashion, the mux device would  be much
>> cleaner.  But in this case, there is a single "back channel" that only
>> needs to be accessed once and can subsequently be ignored.
>
> Well, the EDID needs to be read on every hotplug event, so it's certainly
> not a one-time thing.
>
>> The video
>> driver can grab a lock, call enable_edid(), read out the EDID data into
>> an array, call disable_edid(), release the lock, and that is it.  The
>> other users of that I2C bus can ignore the hidden EDID.
>
> Other I2C users/devices also shouldn't be impacted by the mux; they
> would continue to use the existing I2C APIs for the bus their devices
> are attached to, and not know about the mux.

If other devices that are on the same bus as the EDID don't use the mux, 
how does one ensure that the GPIO is restored to the non-EDID
setting when the display driver is finished?

Perhaps I'm missing something, but it appears to me that the model is to 
set the correct GPIO state before each use, instead of a 
save-set-use-restore model.

In any case, if there is a good way to instantiate the GPIO mux device 
from the device tree, it certainly provides a ready-made solution.  Each 
device that is on the bus in question would have a device node that is a 
child of the GPIO mux node, and the display driver could have a 
phandle-valued property pointing to the mux node, plus a property 
declaring the selection value (or perhaps a single 2-cell property with 
phandle, selection-value).

> The overhead should be
> almost zero; the I2C GPIO mux driver could remember the previous bus
> selection and only modify it (gpio_set_value) when switching between
> busses. I guess there might be a small overhead to taking a lock to
> protect the bus selection; I'm not sure whether the I2C core serializes
> access already or you'd need to add this; check the existing I2C I2C
> bus mux implementation I guess.
>
Mitch Bradley - Jan. 11, 2012, 11:20 p.m.
On 1/11/2012 10:17 AM, Timur Tabi wrote:
> Mitch Bradley wrote:
>
>> I think that would not be a good design.  Presence and location of EDID
>> data is not something that a generic I2C driver should know.  It's the
>> video controller that knows that EDID exists, where it is located
>> (device ID 0x50 and addresses within that device), and what it means.
>
> But the video driver does not know what I2C *bus* it's on.  I have been unable to come up with a way to obtain the proper i2c_adapter object to use when looking for EDID data.

You make device nodes for each of the i2c adapters (possibly with a GPIO 
I2C mux node underneath the EDID one), then you point to the correct 
adapter (or mux) node with a phandle-valued property in the video node.

Of course, you need the deferral patch cited in another message.

>
>>>   That won't work if I have multiple video controllers, since I won't know which EDID data goes with which video controller.  I tried adding a call to i2c_add_driver() in my framebuffer driver, but the I2C probe function was called *after* the framebuffer's probe function, so that doesn't help me.
>>
>> Then there needs to be some sort of ordering or dependency to ensure
>> that the I2C driver is activated before the framebuffer driver tries to
>> use it.
>
> I don't know how to do that, either.
>
>> Either way, you need platform-dependent functions to do the switching,
>> and you need to select the appropriate channel.  Personally, I don't see
>> the advantage of using the mux device in this case.  It's just adding
>> complexity with no payback.
>
> I'm always in favor of doing things the simpler way.
>
>
Timur Tabi - Jan. 11, 2012, 11:32 p.m.
Mitch Bradley wrote:
> You make device nodes for each of the i2c adapters 

We have that already.

> (possibly with a GPIO 
> I2C mux node underneath the EDID one), 

Ok.

> then you point to the correct 
> adapter (or mux) node with a phandle-valued property in the video node.

The only connection between a device tree node and an I2C object is the
of_find_i2c_device_by_node() function.  That requires the I2C device to be
instantiated.

> Of course, you need the deferral patch cited in another message.

Last I heard, it's not ready.  I suspect it will be part of kernel 3.4, at
the earliest.
Stephen Warren - Jan. 12, 2012, 12:15 a.m.
Mitch Bradley wrote at Wednesday, January 11, 2012 4:16 PM:
> On 1/11/2012 10:29 AM, Stephen Warren wrote:
> > Mitch Bradley wrote at Tuesday, January 10, 2012 11:43 PM:
> >> On 1/10/2012 2:28 PM, Timur Tabi wrote:
> >>> Mitch Bradley wrote:
> > ...
> >>>> That GPIO pin thing is annoying, but not sufficiently complex or common
> >>>> that it warrants having a separate EDID driver.  You could define a
> >>>> platform-specific property to tell your framebuffer driver that it needs
> >>>> to do that GPIO thing.  It's a hack, but the GPIO thing is inherently a
> >>>> hack, so there will be some ugliness somewhere as a result.
> >>>
> >>> I have two platform-specific functions, "enabled_edid" and "disable_edid", that I call
> before/after
> >>> calling fb_ddc_read().  This seems to work well, and I already have a mechanism for calling
> platform-
> >>> specific functions from the framebuffer driver.
> >>>
> >>> However, Stephen Warren said I should be using the I2C mux feature instead.
> >>
> >> I2C mux is a plausible solution, as is your enable/disable thing.  At
> >> some level they are equivalent.  I2C mux is a formalization of your
> >> solution, in which the mux device's select method must be written to
> >> perform the function of your enable/disable edid functions.
> >>
> >> Either way, you need platform-dependent functions to do the switching,
> >> and you need to select the appropriate channel.  Personally, I don't see
> >> the advantage of using the mux device in this case.
> >
> > The main advantage I see is that you explicitly don't need any platform-
> > specific functions to do the switching; you end up with platform-agnostic
> > code (the I2C GPIO mux driver) and platform-specific configuration for
> > that driver (the GPIO ID to use).
> 
> 
> Oh, I didn't know about the I2C GPIO Mux driver.  I was looking at
> i2c-mux.c .  I now see gpio-i2cmux.c, which indeed seems to do the right
> thing.
> 
> > The display driver just talks to the
> > I2C API for the DDC I2C bus, and doesn't do anything to switch between
> > the busses; the I2C GPIO mux driver does all that internally. Thus, the
> > display driver will work fine on boards that don't need this muxing with
> > zero changes; the board simply wouldn't register the mux driver.
> >
> >> It's just adding
> >> complexity with no payback.  If there were several channels that needed
> >> to be accessed in an interspersed fashion, the mux device would  be much
> >> cleaner.  But in this case, there is a single "back channel" that only
> >> needs to be accessed once and can subsequently be ignored.
> >
> > Well, the EDID needs to be read on every hotplug event, so it's certainly
> > not a one-time thing.
> >
> >> The video
> >> driver can grab a lock, call enable_edid(), read out the EDID data into
> >> an array, call disable_edid(), release the lock, and that is it.  The
> >> other users of that I2C bus can ignore the hidden EDID.
> >
> > Other I2C users/devices also shouldn't be impacted by the mux; they
> > would continue to use the existing I2C APIs for the bus their devices
> > are attached to, and not know about the mux.
> 
> If other devices that are on the same bus as the EDID don't use the mux,
> how does one ensure that the GPIO is restored to the non-EDID
> setting when the display driver is finished?

The I2C busses are set up like this:

                bus 0        bus 1
I2C controller -------> mux ------> dev a, dev b, dev c, ...
                           \------> dev x, dev y, dev z, ...
                             bus 2

Thus all devices are on a child I2C bus of the mux and none on the raw
HW bus exposed by the I2C controller itself.

The I2C core will always call gpiomux_select before each transaction,
which will set the GPIOs appropriately for bus 1 or bus 2, depending
on which device is being communicated with.

> Perhaps I'm missing something, but it appears to me that the model is to
> set the correct GPIO state before each use, instead of a
> save-set-use-restore model.

That's true, but the select action happens implicitly inside the I2C
core for any and all transactions, AIUI, so the two modes are equivalent.

> In any case, if there is a good way to instantiate the GPIO mux device
> from the device tree, it certainly provides a ready-made solution.  Each
> device that is on the bus in question would have a device node that is a
> child of the GPIO mux node, and the display driver could have a
> phandle-valued property pointing to the mux node, plus a property
> declaring the selection value (or perhaps a single 2-cell property with
> phandle, selection-value).

That's probably the difficult part.

For an I2C mux that is controlled via I2C, you can just add the mux
node as a child of the I2C controller, since it has an I2C address,
and so putting it there makes sense.

But for an I2C mux that's controlled using GPIOs or pinmux, there's no
I2C address so I guess the mux shouldn't be directly underneath the I2C
controller.

Perhaps the DT binding for such an I2C mux can refer to the parent I2C
controller by phandle?

Inside the I2C mux DT node, I think we can have a child node for each
bus, and then use standard I2C child node addressing for all the nodes
within these bus nodes.

Perhaps:

i2c1: i2c@7000c000 {
    #address-cells = <1>;
    #size-cells = <0>;
    compatible = "nvidia,tegra20-i2c";
    reg = <0x7000C000 0x100>;
    interrupts = <0 38 0x04>;
};

mux@0 {
    #address-cells = <1>;
    #size-cells = <0>;
    compatible = "nvidia,tegra20-i2c";
    parent-bus = <&i2c1>;
    gpios = <&gpio 100 0 &gpio 101 0>;
    gpio-values-idle = <0>; /* bitmask of values */

    bus@0 {
        #address-cells = <1>;
        #size-cells = <0>;
        /*
         * The GPIO values to set as a bitmask.
         * Formatted like gpio-i2cmux.c's mux->data.values[i].
         * Or name this gpio-values?
         */
        reg = <1>;

        wm8903: wm8903@1a {
            compatible = "wlf,wm8903";
            reg = <0x1a>;
            ...
        };
    };

    bus@1 {
        #address-cells = <1>;
        #size-cells = <0>;
        reg = <2>;

        light-sensor@44 {
            compatible = "isil,isl29018";
            reg = <0x44>;
            ...
        };
    };
};
Mitch Bradley - Jan. 12, 2012, 12:38 a.m.
On 1/11/2012 2:15 PM, Stephen Warren wrote:
> Mitch Bradley wrote at Wednesday, January 11, 2012 4:16 PM:
>> On 1/11/2012 10:29 AM, Stephen Warren wrote:
>>> Mitch Bradley wrote at Tuesday, January 10, 2012 11:43 PM:
>>>> On 1/10/2012 2:28 PM, Timur Tabi wrote:
>>>>> Mitch Bradley wrote:
>>> ...
>>>>>> That GPIO pin thing is annoying, but not sufficiently complex or common
>>>>>> that it warrants having a separate EDID driver.  You could define a
>>>>>> platform-specific property to tell your framebuffer driver that it needs
>>>>>> to do that GPIO thing.  It's a hack, but the GPIO thing is inherently a
>>>>>> hack, so there will be some ugliness somewhere as a result.
>>>>>
>>>>> I have two platform-specific functions, "enabled_edid" and "disable_edid", that I call
>> before/after
>>>>> calling fb_ddc_read().  This seems to work well, and I already have a mechanism for calling
>> platform-
>>>>> specific functions from the framebuffer driver.
>>>>>
>>>>> However, Stephen Warren said I should be using the I2C mux feature instead.
>>>>
>>>> I2C mux is a plausible solution, as is your enable/disable thing.  At
>>>> some level they are equivalent.  I2C mux is a formalization of your
>>>> solution, in which the mux device's select method must be written to
>>>> perform the function of your enable/disable edid functions.
>>>>
>>>> Either way, you need platform-dependent functions to do the switching,
>>>> and you need to select the appropriate channel.  Personally, I don't see
>>>> the advantage of using the mux device in this case.
>>>
>>> The main advantage I see is that you explicitly don't need any platform-
>>> specific functions to do the switching; you end up with platform-agnostic
>>> code (the I2C GPIO mux driver) and platform-specific configuration for
>>> that driver (the GPIO ID to use).
>>
>>
>> Oh, I didn't know about the I2C GPIO Mux driver.  I was looking at
>> i2c-mux.c .  I now see gpio-i2cmux.c, which indeed seems to do the right
>> thing.
>>
>>> The display driver just talks to the
>>> I2C API for the DDC I2C bus, and doesn't do anything to switch between
>>> the busses; the I2C GPIO mux driver does all that internally. Thus, the
>>> display driver will work fine on boards that don't need this muxing with
>>> zero changes; the board simply wouldn't register the mux driver.
>>>
>>>> It's just adding
>>>> complexity with no payback.  If there were several channels that needed
>>>> to be accessed in an interspersed fashion, the mux device would  be much
>>>> cleaner.  But in this case, there is a single "back channel" that only
>>>> needs to be accessed once and can subsequently be ignored.
>>>
>>> Well, the EDID needs to be read on every hotplug event, so it's certainly
>>> not a one-time thing.
>>>
>>>> The video
>>>> driver can grab a lock, call enable_edid(), read out the EDID data into
>>>> an array, call disable_edid(), release the lock, and that is it.  The
>>>> other users of that I2C bus can ignore the hidden EDID.
>>>
>>> Other I2C users/devices also shouldn't be impacted by the mux; they
>>> would continue to use the existing I2C APIs for the bus their devices
>>> are attached to, and not know about the mux.
>>
>> If other devices that are on the same bus as the EDID don't use the mux,
>> how does one ensure that the GPIO is restored to the non-EDID
>> setting when the display driver is finished?
>
> The I2C busses are set up like this:
>
>                  bus 0        bus 1
> I2C controller ------->  mux ------>  dev a, dev b, dev c, ...
>                             \------>  dev x, dev y, dev z, ...
>                               bus 2
>
> Thus all devices are on a child I2C bus of the mux and none on the raw
> HW bus exposed by the I2C controller itself.
>
> The I2C core will always call gpiomux_select before each transaction,
> which will set the GPIOs appropriately for bus 1 or bus 2, depending
> on which device is being communicated with.
>
>> Perhaps I'm missing something, but it appears to me that the model is to
>> set the correct GPIO state before each use, instead of a
>> save-set-use-restore model.
>
> That's true, but the select action happens implicitly inside the I2C
> core for any and all transactions, AIUI, so the two modes are equivalent.
>
>> In any case, if there is a good way to instantiate the GPIO mux device
>> from the device tree, it certainly provides a ready-made solution.  Each
>> device that is on the bus in question would have a device node that is a
>> child of the GPIO mux node, and the display driver could have a
>> phandle-valued property pointing to the mux node, plus a property
>> declaring the selection value (or perhaps a single 2-cell property with
>> phandle, selection-value).
>
> That's probably the difficult part.
>
> For an I2C mux that is controlled via I2C, you can just add the mux
> node as a child of the I2C controller, since it has an I2C address,
> and so putting it there makes sense.
>
> But for an I2C mux that's controlled using GPIOs or pinmux, there's no
> I2C address so I guess the mux shouldn't be directly underneath the I2C
> controller.
>
> Perhaps the DT binding for such an I2C mux can refer to the parent I2C
> controller by phandle?
>
> Inside the I2C mux DT node, I think we can have a child node for each
> bus, and then use standard I2C child node addressing for all the nodes
> within these bus nodes.
>
> Perhaps:

The scheme below looks good to me, with minor nits picked...

>
> i2c1: i2c@7000c000 {
>      #address-cells =<1>;
>      #size-cells =<0>;
>      compatible = "nvidia,tegra20-i2c";
>      reg =<0x7000C000 0x100>;
>      interrupts =<0 38 0x04>;
> };
>
> mux@0 {
>      #address-cells =<1>;
>      #size-cells =<0>;
>      compatible = "nvidia,tegra20-i2c";

Shouldn't this compatible value be set up to bind to gpio_i2cmux?  The 
node doesn't seem to be hardware-specific.

>      parent-bus =<&i2c1>;
>      gpios =<&gpio 100 0&gpio 101 0>;
>      gpio-values-idle =<0>; /* bitmask of values */
>
>      bus@0 {
>          #address-cells =<1>;
>          #size-cells =<0>;
>          /*
>           * The GPIO values to set as a bitmask.
>           * Formatted like gpio-i2cmux.c's mux->data.values[i].
>           * Or name this gpio-values?
>           */

Did you mean for the comment above to be associated with the 
"gpio-values-idle" property?  It seems out of place here.

>          reg =<1>;

reg =<0>  because this is bus@0

>
>          wm8903: wm8903@1a {
>              compatible = "wlf,wm8903";
>              reg =<0x1a>;
>              ...
>          };
>      };
>
>      bus@1 {
>          #address-cells =<1>;
>          #size-cells =<0>;
>          reg =<2>;

reg =<1> because this is bus@1

>
>          light-sensor@44 {
>              compatible = "isil,isl29018";
>              reg =<0x44>;
>              ...
>          };
>      };
> };
>
Mitch Bradley - Jan. 12, 2012, 12:47 a.m.
More nits picked below ...


 >
 > i2c1: i2c@7000c000 {
 >      #address-cells =<1>;
 >      #size-cells =<0>;
 >      compatible = "nvidia,tegra20-i2c";
 >      reg =<0x7000C000 0x100>;
 >      interrupts =<0 38 0x04>;
 > };
 >
 > mux@0 {

The name "i2cmux" might be more evocative.

 >      #address-cells =<1>;
 >      #size-cells =<0>;
 >      compatible = "nvidia,tegra20-i2c";
 >      parent-bus =<&i2c1>;
 >      gpios =<&gpio 100 0&gpio 101 0>;
 >      gpio-values-idle =<0>; /* bitmask of values */
 >
 >      bus@0 {

Since this implements the i2c bus abstraction, the name should be "i2c"

 >          #address-cells =<1>;
 >          #size-cells =<0>;
 >          /*
 >           * The GPIO values to set as a bitmask.
 >           * Formatted like gpio-i2cmux.c's mux->data.values[i].
 >           * Or name this gpio-values?
 >           */
 >          reg =<1>;
 >
 >          wm8903: wm8903@1a {
 >              compatible = "wlf,wm8903";
 >              reg =<0x1a>;
 >              ...
 >          };
 >      };
 >
 >      bus@1 {

Ditto, name should be "i2c"

 >          #address-cells =<1>;
 >          #size-cells =<0>;
 >          reg =<2>;
 >
 >          light-sensor@44 {
 >              compatible = "isil,isl29018";
 >              reg =<0x44>;
 >              ...
 >          };
 >      };
 > };
Jamie Lokier - Jan. 12, 2012, 12:09 p.m.
Stephen Warren wrote:
> Mitch Bradley wrote at Wednesday, January 11, 2012 4:16 PM:
> > Perhaps I'm missing something, but it appears to me that the model is to
> > set the correct GPIO state before each use, instead of a
> > save-set-use-restore model.
> 
> That's true, but the select action happens implicitly inside the I2C
> core for any and all transactions, AIUI, so the two modes are equivalent.

If there is some reason why it would be desirable to deselect
the DDC I2C bus after transactions, that could be added as an option
to the GPIO I2C mux, but I can't think of a reason.

> For an I2C mux that is controlled via I2C, you can just add the mux
> node as a child of the I2C controller, since it has an I2C address,
> and so putting it there makes sense.

Only if the mux's I2C slave is on the near side of the mux so that
it's always addressable regardless of mux state, and if it's on the
same I2C.

> But for an I2C mux that's controlled using GPIOs or pinmux, there's no
> I2C address so I guess the mux shouldn't be directly underneath the I2C
> controller.

It's connected to: (a) an I2C, and (b) a GPIO.

It can't be child of both, but I don't see why the GPIO can't be
referred by phandle and the I2C it's muxing be the direct parent.
That seems more natural to me.

All the best,
-- Jamie
Jamie Lokier - Jan. 12, 2012, 12:24 p.m.
Stephen Warren wrote:
> Timur Tabi wrote at Wednesday, January 11, 2012 1:33 PM:
> > Stephen Warren wrote:
> > > Well, the EDID needs to be read on every hotplug event, so it's certainly
> > > not a one-time thing.
> > 
> > What is the hotplug event that triggers an EDID read?
> 
> For monitors attached to user-accessible connectors, whenever the user
> physically plugs in a new monitor, you need to read the EDID since it
> could be a new monitor. DVI and HDMI have explicit hotplug detect lines
> to trigger this. I'm not sure how it works for VGA; perhaps you have to
> manually reprobe.

For VGA, some ports have load sensors (same with S-video, TV out).
It's good if those are exposed in the driver :)

Also if you detect DDC1 signal from an old monitor, you'll easily know
when it stops.  Many send DDC1 when nothing is received over DDC2
after a timeout (I vaguely recall 2 seconds or 120 vsyncs.)

If the VGA monitor responds on DDC2 at all (note that surprisingly
many VGA cables are missing the wires so it's not a given), you can
repeatedly probe it with a short command, not even reading anything,
just an address probe.  Or you can read a short range of bytes to
verify it's the same model and serial number.  Be careful in both
cases to prevent contact bounce from turning into an EDID write
though.

On VGA (or DVI-A sometimes) the fact they switch between DDC1 and DDC2
on detecting the I2C clock transitions is, I suspect, why some video
drivers think they need to wiggle the GPIOs first before sending the
EDID transactions.

> Of course, if this thread is talking about built-in LCD displays where
> the connectivity is fixed, this is a non-issue, but it's still a relevant
> general design discussion; hopefully whatever solution that's picked would
> work for DVI/HDMI even if you're dealing with LCDs right now.

Userspace occasionally wants to talk over DDC to DVI/HDMI as well.
Either to retrieve extended EDID, or to send/receive DDC/CI commands,
or to do its own version of the VGA monitor change detection described
above.

For all the above reasons I think the DDC/EDID support in the kernel
should be moving gently towards a (very small) DDC/EDID mini-subsystem
shared by video drivers.  I.e. it should be exposed as a well-known
I2C bus name, and if it's GPIO bit-banging I2C, that should be exposed
as well.

-- Jamie
Stephen Warren - Jan. 12, 2012, 4:45 p.m.
Mitch Bradley wrote at Wednesday, January 11, 2012 5:39 PM:
> On 1/11/2012 2:15 PM, Stephen Warren wrote:
> > Mitch Bradley wrote at Wednesday, January 11, 2012 4:16 PM:
...
> >> In any case, if there is a good way to instantiate the GPIO mux device
> >> from the device tree, it certainly provides a ready-made solution.  Each
> >> device that is on the bus in question would have a device node that is a
> >> child of the GPIO mux node, and the display driver could have a
> >> phandle-valued property pointing to the mux node, plus a property
> >> declaring the selection value (or perhaps a single 2-cell property with
> >> phandle, selection-value).
> >
> > That's probably the difficult part.
> >
> > For an I2C mux that is controlled via I2C, you can just add the mux
> > node as a child of the I2C controller, since it has an I2C address,
> > and so putting it there makes sense.
> >
> > But for an I2C mux that's controlled using GPIOs or pinmux, there's no
> > I2C address so I guess the mux shouldn't be directly underneath the I2C
> > controller.
> >
> > Perhaps the DT binding for such an I2C mux can refer to the parent I2C
> > controller by phandle?
> >
> > Inside the I2C mux DT node, I think we can have a child node for each
> > bus, and then use standard I2C child node addressing for all the nodes
> > within these bus nodes.
> >
> > Perhaps:
> 
> The scheme below looks good to me, with minor nits picked...
> 
> > i2c1: i2c@7000c000 {
> >      #address-cells =<1>;
> >      #size-cells =<0>;
> >      compatible = "nvidia,tegra20-i2c";
> >      reg =<0x7000C000 0x100>;
> >      interrupts =<0 38 0x04>;
> > };
> >
> > mux@0 {
> >      #address-cells =<1>;
> >      #size-cells =<0>;
> >      compatible = "nvidia,tegra20-i2c";
> 
> Shouldn't this compatible value be set up to bind to gpio_i2cmux?  The
> node doesn't seem to be hardware-specific.

Yes, cut/paste mistake.

> >      parent-bus =<&i2c1>;
> >      gpios =<&gpio 100 0&gpio 101 0>;
> >      gpio-values-idle =<0>; /* bitmask of values */
> >
> >      bus@0 {
> >          #address-cells =<1>;
> >          #size-cells =<0>;
> >          /*
> >           * The GPIO values to set as a bitmask.
> >           * Formatted like gpio-i2cmux.c's mux->data.values[i].
> >           * Or name this gpio-values?
> >           */
> 
> Did you mean for the comment above to be associated with the
> "gpio-values-idle" property?  It seems out of place here.

The comment applies to both gpio-values-idle at the top-level, and the
reg values within each of the mux's I2C child busses.

In this binding, I assume that the reg value in the I2C child bus is the
value mux->data.values[i] in the driver code, and gpio-values-idle is
value mux->data.idle. That way, we don't need each child bus to have a
separate property to represent mux->data.values[i]. If we don't want to
overload reg this way, perhaps we could represent child busses as:

i2c@0 {
    reg = <0>; /* arbitrary ID */
    gpio-values = <1>; /* Value determined by HW mux's GPIO values */
    ...
};
i2c@1 {
    reg = <1>;
    gpio-values = <2>;
    ...
};

> >          reg =<1>;
> 
> reg =<0>  because this is bus@0
> 
> >
> >          wm8903: wm8903@1a {
> >              compatible = "wlf,wm8903";
> >              reg =<0x1a>;
> >              ...
> >          };
> >      };
> >
> >      bus@1 {
> >          #address-cells =<1>;
> >          #size-cells =<0>;
> >          reg =<2>;
> 
> reg =<1> because this is bus@1
> 
> >
> >          light-sensor@44 {
> >              compatible = "isil,isl29018";
> >              reg =<0x44>;
> >              ...
> >          };
> >      };
> > };
Stephen Warren - Jan. 12, 2012, 4:49 p.m.
Jamie Lokier wrote at Thursday, January 12, 2012 5:24 AM:
...
> For all the above reasons I think the DDC/EDID support in the kernel
> should be moving gently towards a (very small) DDC/EDID mini-subsystem
> shared by video drivers.  I.e. it should be exposed as a well-known
> I2C bus name, and if it's GPIO bit-banging I2C, that should be exposed
> as well.

I don't think a single well-known bus name would work, since it's very
common for a system to have multiple DDC busses, since they have multiple
video connectors.

However, representing each physical connector as a jack object to user-
space (perhaps related to media controller, or a cross-subsystem extension
of the ALSA jack objects?) and associating the I2C bus ID (or some more
abstract display-oriented ID) with it would make sense.
Stephen Warren - Jan. 12, 2012, 4:52 p.m.
Jamie Lokier wrote at Thursday, January 12, 2012 5:09 AM:
> Stephen Warren wrote:
> > Mitch Bradley wrote at Wednesday, January 11, 2012 4:16 PM:
> > > Perhaps I'm missing something, but it appears to me that the model is to
> > > set the correct GPIO state before each use, instead of a
> > > save-set-use-restore model.
> >
> > That's true, but the select action happens implicitly inside the I2C
> > core for any and all transactions, AIUI, so the two modes are equivalent.
> 
> If there is some reason why it would be desirable to deselect
> the DDC I2C bus after transactions, that could be added as an option
> to the GPIO I2C mux, but I can't think of a reason.

The feature is already in the GPIO I2C mux driver.

> > For an I2C mux that is controlled via I2C, you can just add the mux
> > node as a child of the I2C controller, since it has an I2C address,
> > and so putting it there makes sense.
> 
> Only if the mux's I2C slave is on the near side of the mux so that
> it's always addressable regardless of mux state, and if it's on the
> same I2C.

True. Do any I2C I2C muxes not work like that though?

> > But for an I2C mux that's controlled using GPIOs or pinmux, there's no
> > I2C address so I guess the mux shouldn't be directly underneath the I2C
> > controller.
> 
> It's connected to: (a) an I2C, and (b) a GPIO.
> 
> It can't be child of both, but I don't see why the GPIO can't be
> referred by phandle and the I2C it's muxing be the direct parent.
> That seems more natural to me.

That'd be nice, bus a GPIO I2C mux isn't an addressable device on the
I2C bus; it has no I2C accessible registers or functionality. Since
there's no I2C address, I don't think we can make it a child of the I2C
bus without breaking existing semantics of the reg property for I2C
child nodes.

Patch

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 5ebc5d9..a6e90d5 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -282,6 +282,7 @@  config ARCH_VERSATILE
 
 config ARCH_VEXPRESS
 	bool "ARM Ltd. Versatile Express family"
+	select ARCH_VEXPRESS_SANE_CONFIG
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select ARM_AMBA
 	select ARM_TIMER_SP804
diff --git a/arch/arm/boot/dts/vexpress-v2m-legacy.dtsi b/arch/arm/boot/dts/vexpress-v2m-legacy.dtsi
new file mode 100644
index 0000000..fd6e4e4
--- /dev/null
+++ b/arch/arm/boot/dts/vexpress-v2m-legacy.dtsi
@@ -0,0 +1,163 @@ 
+// ARM Ltd. Versatile Express Motherboard V2M-P1 (HBI-0190D)
+// Legacy memory map
+
+/ {
+	aliases {
+		serial0 = &uart0;
+		serial1 = &uart1;
+		serial2 = &uart2;
+		serial3 = &uart3;
+		i2c0 = &i2c0;
+		i2c1 = &i2c1;
+	};
+
+	motherboard {
+		compatible = "simple-bus";
+		#address-cells = <2>; // SMB chipselect number and offset
+		#size-cells = <1>;
+		#interrupt-cells = <1>;
+
+		flash@0,00000000 {
+			compatible = "arm,vexpress-flash", "cfi-flash";
+			reg = <0 0x00000000 0x04000000
+			       1 0x00000000 0x04000000>;
+			bank-width = <4>;
+		};
+
+		psram@2,00000000 {
+			compatible = "mtd-ram";
+			reg = <2 0x00000000 0x02000000>;
+			bank-width = <4>;
+		};
+
+		ethernet@3,02000000 {
+			compatible = "smsc,lan9118", "smsc,lan9115";
+			reg = <3 0x02000000 0x10000>;
+			reg-io-width = <4>;
+			interrupts = <15>;
+			smsc,irq-active-high;
+			smsc,irq-push-pull;
+		};
+
+		usb@3,03000000 {
+			compatible = "nxp,usb-isp1761";
+			reg = <3 0x03000000 0x20000>;
+			interrupts = <16>;
+			port1-otg;
+		};
+
+		peripherals@7,00000000 {
+			compatible = "arm,amba-bus", "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 7 0 0x20000>;
+
+			// PCI-E I2C bus
+			i2c0: i2c@02000 {
+				compatible = "arm,versatile-i2c";
+				reg = <0x02000 0x1000>;
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				pcie-switch@60 {
+					compatible = "idt,89hpes32h8";
+					reg = <0x60>;
+				};
+			};
+
+			aaci@04000 {
+				compatible = "arm,pl041", "arm,primecell";
+				reg = <0x04000 0x1000>;
+				interrupts = <11>;
+			};
+
+			mmci@05000 {
+				compatible = "arm,pl180", "arm,primecell";
+				reg = <0x05000 0x1000>;
+				interrupts = <9 10>;
+			};
+
+			kmi@06000 {
+				compatible = "arm,pl050", "arm,primecell";
+				reg = <0x06000 0x1000>;
+				interrupts = <12>;
+			};
+
+			kmi@07000 {
+				compatible = "arm,pl050", "arm,primecell";
+				reg = <0x07000 0x1000>;
+				interrupts = <13>;
+			};
+
+			uart0: uart@09000 {
+				compatible = "arm,pl011", "arm,primecell";
+				reg = <0x09000 0x1000>;
+				interrupts = <5>;
+			};
+
+			uart1: uart@0a000 {
+				compatible = "arm,pl011", "arm,primecell";
+				reg = <0x0a000 0x1000>;
+				interrupts = <6>;
+			};
+
+			uart2: uart@0b000 {
+				compatible = "arm,pl011", "arm,primecell";
+				reg = <0x0b000 0x1000>;
+				interrupts = <7>;
+			};
+
+			uart3: uart@0c000 {
+				compatible = "arm,pl011", "arm,primecell";
+				reg = <0x0c000 0x1000>;
+				interrupts = <8>;
+			};
+
+			wdt@0f000 {
+				compatible = "arm,sp805", "arm,primecell";
+				reg = <0x0f000 0x1000>;
+				interrupts = <0>;
+			};
+
+			// Timer init is hardcoded in v2m_timer_init(), for now.
+			// timer@11000 {
+			//	compatible = "arm,arm-sp804";
+			//	reg = <0x11000 0x1000>;
+			//	interrupts = <2>;
+			// };
+
+			// timer@12000 {
+			//	compatible = "arm,arm-sp804";
+			//	reg = <0x12000 0x1000>;
+			// };
+
+			// DVI I2C bus (DDC)
+			i2c1: i2c@16000 {
+				compatible = "arm,versatile-i2c";
+				reg = <0x16000 0x1000>;
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				edid@50 {
+					compatible = "edid";
+					reg = <0x50>;
+				};
+			};
+
+			rtc@17000 {
+				compatible = "arm,pl031", "arm,primecell";
+				reg = <0x017000 0x1000>;
+				interrupts = <4>;
+			};
+
+			compact-flash@1a000 {
+				compatible = "ata-generic";
+				reg = <0x1a000 0x100
+				       0x1a100 0xf00>;
+				reg-shift = <2>;
+			};
+		};
+	};
+};
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
new file mode 100644
index 0000000..059be97
--- /dev/null
+++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
@@ -0,0 +1,80 @@ 
+// ARM Ltd. Versatile Express Corex-A9 (Quad Core) Core Tile V2P-CA9 (HBI-0191B)
+
+/dts-v1/;
+
+/include/ "skeleton.dtsi"
+
+/ {
+	model = "ARM Versatile Express (Cortex-A9 Quad Core Tile)";
+	compatible = "arm,vexpress-v2p-ca9", "arm,vexpress";
+	interrupt-parent = <&intc>;
+
+	memory {
+		device_type = "memory";
+		reg = <0x60000000 0x40000000>;
+	};
+
+	intc: interrupt-controller@1e001000 {
+		compatible = "arm,cortex-a9-gic";
+		#interrupt-cells = <2>;
+		#address-cells = <0>;
+		interrupt-controller;
+		reg = <0x1e001000 0x1000>,
+		      <0x1e000100 0x100>;
+	};
+
+	motherboard {
+		ranges = <0 0 0x40000000 0x04000000
+			  1 0 0x44000000 0x04000000
+			  2 0 0x48000000 0x04000000
+			  3 0 0x4c000000 0x04000000
+			  7 0 0x10000000 0x00020000>;
+
+		interrupt-map-mask = <0 0 63>;
+		interrupt-map = <0 0 0 &intc 32 8
+				 0 0 1 &intc 33 4
+				 0 0 2 &intc 34 4
+				 0 0 3 &intc 35 4
+				 0 0 4 &intc 36 4
+				 0 0 5 &intc 37 4
+				 0 0 6 &intc 38 4
+				 0 0 7 &intc 39 4
+				 0 0 8 &intc 40 4
+				 0 0 9 &intc 41 4
+				 0 0 10 &intc 42 4
+				 0 0 11 &intc 43 4
+				 0 0 12 &intc 44 4
+				 0 0 13 &intc 45 4
+				 0 0 14 &intc 46 4
+				 0 0 15 &intc 47 4
+				 0 0 16 &intc 48 4
+				 0 0 17 &intc 49 4
+				 0 0 18 &intc 50 4
+				 0 0 19 &intc 51 4
+				 0 0 20 &intc 52 4
+				 0 0 21 &intc 53 4
+				 0 0 22 &intc 54 4
+				 0 0 23 &intc 55 4
+				 0 0 24 &intc 56 4
+				 0 0 25 &intc 57 4
+				 0 0 26 &intc 58 4
+				 0 0 27 &intc 59 4
+				 0 0 28 &intc 60 4
+				 0 0 29 &intc 61 4
+				 0 0 30 &intc 62 4
+				 0 0 31 &intc 63 4
+				 0 0 32 &intc 64 4
+				 0 0 33 &intc 65 4
+				 0 0 34 &intc 66 4
+				 0 0 35 &intc 67 4
+				 0 0 36 &intc 68 4
+				 0 0 37 &intc 69 4
+				 0 0 38 &intc 70 4
+				 0 0 39 &intc 71 4
+				 0 0 40 &intc 72 4
+				 0 0 41 &intc 73 4
+				 0 0 42 &intc 74 4>;
+	};
+};
+
+/include/ "vexpress-v2m-legacy.dtsi"
diff --git a/arch/arm/configs/vexpress_defconfig b/arch/arm/configs/vexpress_defconfig
index f2de51f..6c3c5f6 100644
--- a/arch/arm/configs/vexpress_defconfig
+++ b/arch/arm/configs/vexpress_defconfig
@@ -22,6 +22,7 @@  CONFIG_MODULE_UNLOAD=y
 # CONFIG_IOSCHED_DEADLINE is not set
 # CONFIG_IOSCHED_CFQ is not set
 CONFIG_ARCH_VEXPRESS=y
+CONFIG_ARCH_VEXPRESS_ATAGS=y
 CONFIG_ARCH_VEXPRESS_CA9X4=y
 # CONFIG_SWP_EMULATE is not set
 CONFIG_SMP=y
diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
index 9311484..ea64630 100644
--- a/arch/arm/mach-vexpress/Kconfig
+++ b/arch/arm/mach-vexpress/Kconfig
@@ -1,12 +1,55 @@ 
 menu "Versatile Express platform type"
 	depends on ARCH_VEXPRESS
 
+# ARCH_VEXPRESS ensures a sane minimal config is selected by selecting
+# ARCH_VEXPRESS_SANE_CONFIG.
+# Extend the logic here when adding new core tiles.
+
+config ARCH_VEXPRESS_SANE_CONFIG
+	bool
+	select ARCH_VEXPRESS_CA9X4
+	select ARCH_VEXPRESS_ATAGS if !ARCH_VEXPRESS_DT
+
+
+comment "At least one boot type must be selected"
+
+config ARCH_VEXPRESS_ATAGS
+	bool "Boot via ATAGs"
+	default y
+	help
+	  This option enables support for the board using the standard
+	  ATAGs boot protocol.
+
+	  If your bootloader supports FDT-based booting and you do not
+	  intend ever to boot via the traditional ATAGs method, you can say
+	  N here.
+
+config ARCH_VEXPRESS_DT
+	bool "Boot via Device Tree"
+	select USE_OF
+	help
+	  This option enables support for the board, and enables booting
+	  via a Flattened Device Tree provided by the bootloader.
+
+	  If your bootloader supports FDT-based booting, you can say Y
+	  here, otherwise, say N.
+
+
+# Core Tile support options
+
+comment "At least one core tile must be selected"
+
 config ARCH_VEXPRESS_CA9X4
-	bool "Versatile Express Cortex-A9x4 tile"
+	bool "Versatile Express Cortex-A9x4 Core Tile"
+	default y
 	select CPU_V7
 	select ARM_GIC
 	select ARM_ERRATA_720789
 	select ARM_ERRATA_751472
 	select ARM_ERRATA_753970
+	help
+	  Include support for the Cortex-A9x4 Core Tile (HBI-0191B).
+
+	  If unsure, say Y.
 
 endmenu
diff --git a/arch/arm/mach-vexpress/ct-ca9x4.c b/arch/arm/mach-vexpress/ct-ca9x4.c
index bfd32f5..e2fe2c9 100644
--- a/arch/arm/mach-vexpress/ct-ca9x4.c
+++ b/arch/arm/mach-vexpress/ct-ca9x4.c
@@ -9,6 +9,7 @@ 
 #include <linux/amba/bus.h>
 #include <linux/amba/clcd.h>
 #include <linux/clkdev.h>
+#include <linux/irqdomain.h>
 
 #include <asm/hardware/arm_timer.h>
 #include <asm/hardware/cache-l2x0.h>
@@ -59,10 +60,16 @@  static void __init ct_ca9x4_map_io(void)
 	iotable_init(ct_ca9x4_io_desc, ARRAY_SIZE(ct_ca9x4_io_desc));
 }
 
+static const struct of_device_id gic_of_match[] __initconst = {
+	{ .compatible = "arm,cortex-a9-gic", },
+	{}
+};
+
 static void __init ct_ca9x4_init_irq(void)
 {
 	gic_init(0, 29, MMIO_P2V(A9_MPCORE_GIC_DIST),
 		 MMIO_P2V(A9_MPCORE_GIC_CPU));
+	irq_domain_generate_simple(gic_of_match, A9_MPCORE_GIC_DIST, 0);
 }
 
 #if 0
diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index 9e6b93b..6defce6 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -6,6 +6,8 @@ 
 #include <linux/amba/mmci.h>
 #include <linux/io.h>
 #include <linux/init.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/ata_platform.h>
 #include <linux/smsc911x.h>
@@ -118,7 +120,7 @@  int v2m_cfg_read(u32 devfn, u32 *data)
 	return !!(val & SYS_CFG_ERR);
 }
 
-
+#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
 static struct resource v2m_pcie_i2c_resource = {
 	.start	= V2M_SERIAL_BUS_PCI,
 	.end	= V2M_SERIAL_BUS_PCI + SZ_4K - 1,
@@ -200,6 +202,7 @@  static struct platform_device v2m_usb_device = {
 	.num_resources	= ARRAY_SIZE(v2m_usb_resources),
 	.dev.platform_data = &v2m_usb_config,
 };
+#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
 
 static void v2m_flash_set_vpp(struct platform_device *pdev, int on)
 {
@@ -211,6 +214,7 @@  static struct physmap_flash_data v2m_flash_data = {
 	.set_vpp	= v2m_flash_set_vpp,
 };
 
+#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
 static struct resource v2m_flash_resources[] = {
 	{
 		.start	= V2M_NOR0,
@@ -254,6 +258,7 @@  static struct platform_device v2m_cf_device = {
 	.num_resources	= ARRAY_SIZE(v2m_pata_resources),
 	.dev.platform_data = &v2m_pata_data,
 };
+#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
 
 static unsigned int v2m_mmci_status(struct device *dev)
 {
@@ -265,6 +270,7 @@  static struct mmci_platform_data v2m_mmci_data = {
 	.status		= v2m_mmci_status,
 };
 
+#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
 static AMBA_DEVICE(aaci,  "mb:aaci",  V2M_AACI, NULL);
 static AMBA_DEVICE(mmci,  "mb:mmci",  V2M_MMCI, &v2m_mmci_data);
 static AMBA_DEVICE(kmi0,  "mb:kmi0",  V2M_KMI0, NULL);
@@ -288,6 +294,7 @@  static struct amba_device *v2m_amba_devs[] __initdata = {
 	&wdt_device,
 	&rtc_device,
 };
+#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
 
 
 static long v2m_osc_round(struct clk *clk, unsigned long rate)
@@ -415,6 +422,8 @@  static void __init v2m_init_irq(void)
 	ct_desc->init_irq();
 }
 
+
+#ifdef CONFIG_ARCH_VEXPRESS_ATAGS
 static void __init v2m_init(void)
 {
 	int i;
@@ -443,3 +452,46 @@  MACHINE_START(VEXPRESS, "ARM-Versatile Express")
 	.timer		= &v2m_timer,
 	.init_machine	= v2m_init,
 MACHINE_END
+#endif /* CONFIG_ARCH_VEXPRESS_ATAGS */
+
+#ifdef CONFIG_ARCH_VEXPRESS_DT
+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_AACI, "mb:aaci", NULL),
+	OF_DEV_AUXDATA("arm,primecell", V2M_WDT, "mb:wdt", NULL),
+	OF_DEV_AUXDATA("arm,primecell", V2M_MMCI, "mb:mmci", &v2m_mmci_data),
+	OF_DEV_AUXDATA("arm,primecell", V2M_KMI0, "mb:kmi0", NULL),
+	OF_DEV_AUXDATA("arm,primecell", V2M_KMI1, "mb:kmi1", NULL),
+	OF_DEV_AUXDATA("arm,primecell", V2M_UART0, "mb:uart0", NULL),
+	OF_DEV_AUXDATA("arm,primecell", V2M_UART1, "mb:uart1", NULL),
+	OF_DEV_AUXDATA("arm,primecell", V2M_UART2, "mb:uart2", NULL),
+	OF_DEV_AUXDATA("arm,primecell", V2M_UART3, "mb:uart3", NULL),
+	OF_DEV_AUXDATA("arm,primecell", V2M_RTC, "mb:rtc", NULL),
+	{}
+};
+
+static void __init v2m_dt_init(void)
+{
+	of_platform_populate(NULL, of_default_bus_match_table,
+			     v2m_dt_auxdata_lookup, NULL);
+
+	pm_power_off = v2m_power_off;
+	arm_pm_restart = v2m_restart;
+
+	ct_desc->init_tile();
+}
+
+static const char *v2m_dt_match[] __initconst = {
+	"arm,vexpress",
+	NULL,
+};
+
+DT_MACHINE_START(VEXPRESS_DT, "ARM Versatile Express")
+	.map_io		= v2m_map_io,
+	.init_early	= v2m_init_early,
+	.init_irq	= v2m_init_irq,
+	.timer		= &v2m_timer,
+	.init_machine	= v2m_dt_init,
+	.dt_compat	= v2m_dt_match,
+MACHINE_END
+#endif /* CONFIG_ARCH_VEXPRESS_DT */