| Message ID | 1316596786-2539-1-git-send-email-dave.martin@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
> 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ł
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.
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
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ł
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
> > 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ł
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
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
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 >
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
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.
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.
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.
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
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
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.
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. > >
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.
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.
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.
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.
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.
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.
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?
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.
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.
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.
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. >
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. > >
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.
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>; ... }; }; };
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>; > ... > }; > }; > }; >
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>; > ... > }; > }; > };
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
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
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>; > > ... > > }; > > }; > > };
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.
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.
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 */