[U-Boot] [PATCH 0/6] sunxi: HDMI (cfb) console support

[U-Boot] [PATCH 0/6] sunxi: HDMI (cfb) console support

[Hans de Goede]

Hi all,

Now that the simplefb devicetree binding stuff has finally been sorted out,
I would like to move forward with this patch-set.

Ian, can you please review the entire set, from the sunxi pov ?

Anatolij, can you please review the "sunxi: video: Add cfb console driver
for sunxi" patch from a video subsys pov, it uses the cfb stuff as is,
without making any changes, so there should not be any problems, but still
I would like your acked by for this.

If the review goes smoothly I would still like to get this series into
v2015.01, the initial version of these patches have been posted before the
previous merge window, so this patch-set has been long enough in the making
already. It has been in my personal tree and used when ever I have been doing
sunxi work for months now, without any issues.

Regards,

Hans

[U-Boot] [PATCH 1/6] sun4i: Rename dram_clk_cfg to dram_clk_gate

[Hans de Goede]

The data sheet just calls it DRAM_CLK_REG, and on sun6i we've both a
dram_clk_cfg and dram_clk_gate, and the sun4i reg matches dram_clk_gate on
sun6i, so name it the same on sun4i.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/cpu/armv7/sunxi/dram_sun4i.c         | 4 ++--
 arch/arm/include/asm/arch-sunxi/clock_sun4i.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
index dc9fdb9..ec8aaa7 100644
--- a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
+++ b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
@@ -428,9 +428,9 @@ static void dramc_clock_output_en(u32 on)
 #ifdef CONFIG_MACH_SUN4I
 	struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
 	if (on)
-		setbits_le32(&ccm->dram_clk_cfg, CCM_DRAM_CTRL_DCLK_OUT);
+		setbits_le32(&ccm->dram_clk_gate, CCM_DRAM_CTRL_DCLK_OUT);
 	else
-		clrbits_le32(&ccm->dram_clk_cfg, CCM_DRAM_CTRL_DCLK_OUT);
+		clrbits_le32(&ccm->dram_clk_gate, CCM_DRAM_CTRL_DCLK_OUT);
 #endif
 }
 
diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
index 9dca800..6c0430c 100644
--- a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
+++ b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
@@ -62,7 +62,7 @@ struct sunxi_ccm_reg {
 	u32 gps_clk_cfg;	/* 0xd0 */
 	u32 spi3_clk_cfg;	/* 0xd4 */
 	u8 res5[0x28];
-	u32 dram_clk_cfg;	/* 0x100 */
+	u32 dram_clk_gate;	/* 0x100 */
 	u32 be0_clk_cfg;	/* 0x104 */
 	u32 be1_clk_cfg;	/* 0x108 */
 	u32 fe0_clk_cfg;	/* 0x10c */
-- 
2.1.0

[U-Boot] [PATCH 2/6] sunxi: Add video pll clock functions

[Hans de Goede]

This is a preparation patch for adding support for HDMI out.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/cpu/armv7/sunxi/clock_sun4i.c        | 27 ++++++++++++
 arch/arm/cpu/armv7/sunxi/clock_sun6i.c        | 29 +++++++++++++
 arch/arm/include/asm/arch-sunxi/clock.h       |  2 +
 arch/arm/include/asm/arch-sunxi/clock_sun4i.h | 45 ++++++++++++++++++++
 arch/arm/include/asm/arch-sunxi/clock_sun6i.h | 59 ++++++++++++++++++++++++++-
 5 files changed, 161 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/armv7/sunxi/clock_sun4i.c b/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
index a0e49d1..49f4032 100644
--- a/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
+++ b/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
@@ -180,6 +180,21 @@ void clock_set_pll1(unsigned int hz)
 }
 #endif
 
+void clock_set_pll3(unsigned int clk)
+{
+	struct sunxi_ccm_reg * const ccm =
+		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+	if (clk == 0) {
+		clrbits_le32(&ccm->pll3_cfg, CCM_PLL3_CTRL_EN);
+		return;
+	}
+
+	/* PLL3 rate = 3000000 * m */
+	writel(CCM_PLL3_CTRL_EN | CCM_PLL3_CTRL_INTEGER_MODE |
+	       CCM_PLL3_CTRL_M(clk / 3000000), &ccm->pll3_cfg);
+}
+
 unsigned int clock_get_pll5p(void)
 {
 	struct sunxi_ccm_reg *const ccm =
@@ -200,3 +215,15 @@ unsigned int clock_get_pll6(void)
 	int k = ((rval & CCM_PLL6_CTRL_K_MASK) >> CCM_PLL6_CTRL_K_SHIFT) + 1;
 	return 24000000 * n * k / 2;
 }
+
+void clock_set_de_mod_clock(u32 *clk_cfg, unsigned int hz)
+{
+	int pll = clock_get_pll5p();
+	int div = 1;
+
+	while ((pll / div) > hz)
+		div++;
+
+	writel(CCM_DE_CTRL_GATE | CCM_DE_CTRL_RST | CCM_DE_CTRL_PLL5P |
+	       CCM_DE_CTRL_M(div), clk_cfg);
+}
diff --git a/arch/arm/cpu/armv7/sunxi/clock_sun6i.c b/arch/arm/cpu/armv7/sunxi/clock_sun6i.c
index 16ab6f3..8e949c6 100644
--- a/arch/arm/cpu/armv7/sunxi/clock_sun6i.c
+++ b/arch/arm/cpu/armv7/sunxi/clock_sun6i.c
@@ -127,6 +127,23 @@ void clock_set_pll1(unsigned int clk)
 }
 #endif
 
+void clock_set_pll3(unsigned int clk)
+{
+	struct sunxi_ccm_reg * const ccm =
+		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+	const int m = 8; /* 3 MHz steps just like sun4i, sun5i and sun7i */
+
+	if (clk == 0) {
+		clrbits_le32(&ccm->pll3_cfg, CCM_PLL3_CTRL_EN);
+		return;
+	}
+
+	/* PLL3 rate = 24000000 * n / m */
+	writel(CCM_PLL3_CTRL_EN | CCM_PLL3_CTRL_INTEGER_MODE |
+	       CCM_PLL3_CTRL_N(clk / (24000000 / m)) | CCM_PLL3_CTRL_M(m),
+	       &ccm->pll3_cfg);
+}
+
 void clock_set_pll5(unsigned int clk)
 {
 	struct sunxi_ccm_reg * const ccm =
@@ -151,3 +168,15 @@ unsigned int clock_get_pll6(void)
 	int k = ((rval & CCM_PLL6_CTRL_K_MASK) >> CCM_PLL6_CTRL_K_SHIFT) + 1;
 	return 24000000 * n * k / 2;
 }
+
+void clock_set_de_mod_clock(u32 *clk_cfg, unsigned int hz)
+{
+	int pll = clock_get_pll6() * 2;
+	int div = 1;
+
+	while ((pll / div) > hz)
+		div++;
+
+	writel(CCM_DE_CTRL_GATE | CCM_DE_CTRL_PLL6_2X | CCM_DE_CTRL_M(div),
+	       clk_cfg);
+}
diff --git a/arch/arm/include/asm/arch-sunxi/clock.h b/arch/arm/include/asm/arch-sunxi/clock.h
index b40c16b..64acff3 100644
--- a/arch/arm/include/asm/arch-sunxi/clock.h
+++ b/arch/arm/include/asm/arch-sunxi/clock.h
@@ -25,9 +25,11 @@
 int clock_init(void);
 int clock_twi_onoff(int port, int state);
 void clock_set_pll1(unsigned int hz);
+void clock_set_pll3(unsigned int hz);
 void clock_set_pll5(unsigned int hz);
 unsigned int clock_get_pll5p(void);
 unsigned int clock_get_pll6(void);
+void clock_set_de_mod_clock(u32 *clk_cfg, unsigned int hz);
 void clock_init_safe(void);
 void clock_init_uart(void);
 #endif
diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
index 6c0430c..eb88969 100644
--- a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
+++ b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
@@ -186,12 +186,20 @@ struct sunxi_ccm_reg {
 
 /* ahb clock gate bit offset (second register) */
 #define AHB_GATE_OFFSET_GMAC		17
+#define AHB_GATE_OFFSET_DE_BE0		12
+#define AHB_GATE_OFFSET_HDMI		11
+#define AHB_GATE_OFFSET_LCD1		5
+#define AHB_GATE_OFFSET_LCD0		4
 
 #define CCM_AHB_GATE_GPS (0x1 << 26)
 #define CCM_AHB_GATE_SDRAM (0x1 << 14)
 #define CCM_AHB_GATE_DLL (0x1 << 15)
 #define CCM_AHB_GATE_ACE (0x1 << 16)
 
+#define CCM_PLL3_CTRL_M(n)		(((n) & 0x7f) << 0)
+#define CCM_PLL3_CTRL_INTEGER_MODE	(0x1 << 15)
+#define CCM_PLL3_CTRL_EN		(0x1 << 31)
+
 #define CCM_PLL5_CTRL_M(n) (((n) & 0x3) << 0)
 #define CCM_PLL5_CTRL_M_MASK CCM_PLL5_CTRL_M(0x3)
 #define CCM_PLL5_CTRL_M_X(n) ((n) - 1)
@@ -253,6 +261,34 @@ struct sunxi_ccm_reg {
 
 #define CCM_MMC_CTRL_ENABLE (0x1 << 31)
 
+#define CCM_DRAM_GATE_OFFSET_DE_BE0	26
+
+#define CCM_LCD_CH0_CTRL_PLL3		(0 << 24)
+#define CCM_LCD_CH0_CTRL_PLL7		(1 << 24)
+#define CCM_LCD_CH0_CTRL_PLL3_2X	(2 << 24)
+#define CCM_LCD_CH0_CTRL_PLL7_2X	(3 << 24)
+#define CCM_LCD_CH0_CTRL_RST		(0x1 << 30)
+#define CCM_LCD_CH0_CTRL_GATE		(0x1 << 31)
+
+#define CCM_LCD_CH1_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
+/* We leave bit 11 set to 0, so sclk1 == sclk2 */
+#define CCM_LCD_CH1_CTRL_PLL3		(0 << 24)
+#define CCM_LCD_CH1_CTRL_PLL7		(1 << 24)
+#define CCM_LCD_CH1_CTRL_PLL3_2X	(2 << 24)
+#define CCM_LCD_CH1_CTRL_PLL7_2X	(3 << 24)
+/* Enable / disable both ch1 sclk1 and sclk2@the same time */
+#define CCM_LCD_CH1_CTRL_GATE		(0x1 << 31 | 0x1 << 15)
+
+#define CCM_HDMI_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
+#define CCM_HDMI_CTRL_PLL_MASK		(3 << 24)
+#define CCM_HDMI_CTRL_PLL3		(0 << 24)
+#define CCM_HDMI_CTRL_PLL7		(1 << 24)
+#define CCM_HDMI_CTRL_PLL3_2X		(2 << 24)
+#define CCM_HDMI_CTRL_PLL7_2X		(3 << 24)
+/* No separate ddc gate on sun4i, sun5i and sun7i */
+#define CCM_HDMI_CTRL_DDC_GATE		0
+#define CCM_HDMI_CTRL_GATE		(0x1 << 31)
+
 #define CCM_GMAC_CTRL_TX_CLK_SRC_MII 0x0
 #define CCM_GMAC_CTRL_TX_CLK_SRC_EXT_RGMII 0x1
 #define CCM_GMAC_CTRL_TX_CLK_SRC_INT_RGMII 0x2
@@ -266,4 +302,13 @@ struct sunxi_ccm_reg {
 #define CCM_USB_CTRL_PHY1_CLK 0
 #define CCM_USB_CTRL_PHY2_CLK 0
 
+/* CCM bits common to all Display Engine (and IEP) clock ctrl regs */
+#define CCM_DE_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
+#define CCM_DE_CTRL_PLL_MASK		(3 << 24)
+#define CCM_DE_CTRL_PLL3		(0 << 24)
+#define CCM_DE_CTRL_PLL7		(1 << 24)
+#define CCM_DE_CTRL_PLL5P		(2 << 24)
+#define CCM_DE_CTRL_RST			(1 << 30)
+#define CCM_DE_CTRL_GATE		(1 << 31)
+
 #endif /* _SUNXI_CLOCK_SUN4I_H */
diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
index e16a764..50a4b69 100644
--- a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
+++ b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
@@ -176,13 +176,18 @@ struct sunxi_ccm_reg {
 #define CCM_PLL1_CTRL_MAGIC		(0x1 << 16)
 #define CCM_PLL1_CTRL_EN		(0x1 << 31)
 
+#define CCM_PLL3_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
+#define CCM_PLL3_CTRL_N(n)		((((n) - 1) & 0x7f) << 8)
+#define CCM_PLL3_CTRL_INTEGER_MODE	(0x1 << 24)
+#define CCM_PLL3_CTRL_EN		(0x1 << 31)
+
 #define CCM_PLL5_CTRL_M(n)		((((n) - 1) & 0x3) << 0)
 #define CCM_PLL5_CTRL_K(n)		((((n) - 1) & 0x3) << 4)
 #define CCM_PLL5_CTRL_N(n)		((((n) - 1) & 0x1f) << 8)
 #define CCM_PLL5_CTRL_UPD		(0x1 << 20)
 #define CCM_PLL5_CTRL_EN		(0x1 << 31)
 
-#define PLL6_CFG_DEFAULT		0x90041811
+#define PLL6_CFG_DEFAULT		0x90041811 /* 600 MHz */
 
 #define CCM_PLL6_CTRL_N_SHIFT		8
 #define CCM_PLL6_CTRL_N_MASK		(0x1f << CCM_PLL6_CTRL_N_SHIFT)
@@ -193,6 +198,7 @@ struct sunxi_ccm_reg {
 
 #define AXI_GATE_OFFSET_DRAM		0
 
+/* ahb_gate0 offsets */
 #define AHB_GATE_OFFSET_USB_OHCI1	30
 #define AHB_GATE_OFFSET_USB_OHCI0	29
 #define AHB_GATE_OFFSET_USB_EHCI1	27
@@ -204,6 +210,13 @@ struct sunxi_ccm_reg {
 #define AHB_GATE_OFFSET_MMC0		8
 #define AHB_GATE_OFFSET_MMC(n)		(AHB_GATE_OFFSET_MMC0 + (n))
 
+/* ahb_gate1 offsets */
+#define AHB_GATE_OFFSET_DRC0		25
+#define AHB_GATE_OFFSET_DE_BE0		12
+#define AHB_GATE_OFFSET_HDMI		11
+#define AHB_GATE_OFFSET_LCD1		5
+#define AHB_GATE_OFFSET_LCD0		4
+
 #define CCM_MMC_CTRL_OSCM24 (0x0 << 24)
 #define CCM_MMC_CTRL_PLL6   (0x1 << 24)
 
@@ -223,8 +236,34 @@ struct sunxi_ccm_reg {
 #define CCM_DRAMCLK_CFG_UPD		(0x1 << 16)
 #define CCM_DRAMCLK_CFG_RST		(0x1 << 31)
 
+#define CCM_DRAM_GATE_OFFSET_DE_BE0	26
+
+#define CCM_LCD_CH0_CTRL_PLL3		(0 << 24)
+#define CCM_LCD_CH0_CTRL_PLL7		(1 << 24)
+#define CCM_LCD_CH0_CTRL_PLL3_2X	(2 << 24)
+#define CCM_LCD_CH0_CTRL_PLL7_2X	(3 << 24)
+#define CCM_LCD_CH0_CTRL_MIPI_PLL	(4 << 24)
+#define CCM_LCD_CH0_CTRL_GATE		(0x1 << 31)
+
+#define CCM_LCD_CH1_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
+#define CCM_LCD_CH1_CTRL_PLL3		(0 << 24)
+#define CCM_LCD_CH1_CTRL_PLL7		(1 << 24)
+#define CCM_LCD_CH1_CTRL_PLL3_2X	(2 << 24)
+#define CCM_LCD_CH1_CTRL_PLL7_2X	(3 << 24)
+#define CCM_LCD_CH1_CTRL_GATE		(0x1 << 31)
+
+#define CCM_HDMI_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
+#define CCM_HDMI_CTRL_PLL_MASK		(3 << 24)
+#define CCM_HDMI_CTRL_PLL3		(0 << 24)
+#define CCM_HDMI_CTRL_PLL7		(1 << 24)
+#define CCM_HDMI_CTRL_PLL3_2X		(2 << 24)
+#define CCM_HDMI_CTRL_PLL7_2X		(3 << 24)
+#define CCM_HDMI_CTRL_DDC_GATE		(0x1 << 30)
+#define CCM_HDMI_CTRL_GATE		(0x1 << 31)
+
 #define MBUS_CLK_DEFAULT		0x81000001 /* PLL6 / 2 */
 
+/* ahb_reset0 offsets */
 #define AHB_RESET_OFFSET_MCTL		14
 #define AHB_RESET_OFFSET_MMC3		11
 #define AHB_RESET_OFFSET_MMC2		10
@@ -232,10 +271,28 @@ struct sunxi_ccm_reg {
 #define AHB_RESET_OFFSET_MMC0		8
 #define AHB_RESET_OFFSET_MMC(n)		(AHB_RESET_OFFSET_MMC0 + (n))
 
+/* ahb_reset0 offsets */
+#define AHB_RESET_OFFSET_DRC0		25
+#define AHB_RESET_OFFSET_DE_BE0		12
+#define AHB_RESET_OFFSET_HDMI		11
+#define AHB_RESET_OFFSET_LCD1		5
+#define AHB_RESET_OFFSET_LCD0		4
+
 /* apb2 reset */
 #define APB2_RESET_UART_SHIFT		(16)
 #define APB2_RESET_UART_MASK		(0xff << APB2_RESET_UART_SHIFT)
 #define APB2_RESET_TWI_SHIFT		(0)
 #define APB2_RESET_TWI_MASK		(0xf << APB2_RESET_TWI_SHIFT)
 
+/* CCM bits common to all Display Engine (and IEP) clock ctrl regs */
+#define CCM_DE_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
+#define CCM_DE_CTRL_PLL_MASK		(0xf << 24)
+#define CCM_DE_CTRL_PLL3		(0 << 24)
+#define CCM_DE_CTRL_PLL7		(1 << 24)
+#define CCM_DE_CTRL_PLL6_2X		(2 << 24)
+#define CCM_DE_CTRL_PLL8		(3 << 24)
+#define CCM_DE_CTRL_PLL9		(4 << 24)
+#define CCM_DE_CTRL_PLL10		(5 << 24)
+#define CCM_DE_CTRL_GATE		(1 << 31)
+
 #endif /* _SUNXI_CLOCK_SUN6I_H */
-- 
2.1.0

[U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi

[Hans de Goede]

From: Luc Verhaegen <libv@skynet.be>

This adds a fixed mode hdmi driver for the sunxi platform. The fixed
mode is a relatively safe 1024x768, more complete EDID handling is
currently not provided. Only HDMI is supported today.

This code is enabled when HPD detects an attached monitor.

Current config is such that 8MB is shaved off at the top of the RAM.
This avoids several memory handling issues, most significant is the fact
that on linux on ARM you are not allowed to remap known RAM as IO. A
clued in display driver will be able to recycle this reserved RAM in
future though.

cfbconsole was chosen as it provides the most important functionality: a
working u-boot console, allowing for the debugging of certain issues
without the need for a UART.

Signed-off-by: Luc Verhaegen <libv@skynet.be>
[hdegoede at redhat.com: Major cleanups and some small bugfixes]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/include/asm/arch-sunxi/display.h | 198 +++++++++++++++
 board/sunxi/Kconfig                       |   7 +
 configs/Ippo_q8h_v5_defconfig             |   1 +
 drivers/video/Makefile                    |   1 +
 drivers/video/sunxi_display.c             | 387 ++++++++++++++++++++++++++++++
 include/configs/sunxi-common.h            |  34 +++
 6 files changed, 628 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-sunxi/display.h
 create mode 100644 drivers/video/sunxi_display.c

diff --git a/arch/arm/include/asm/arch-sunxi/display.h b/arch/arm/include/asm/arch-sunxi/display.h
new file mode 100644
index 0000000..8ac7d1b
--- /dev/null
+++ b/arch/arm/include/asm/arch-sunxi/display.h
@@ -0,0 +1,198 @@
+/*
+ * Sunxi platform display cntroller register and constant defines
+ *
+ * (C) Copyright 2014 Hans de Goede <hdegoede@redhat.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef _SUNXI_DISPLAY_H
+#define _SUNXI_DISPLAY_H
+
+struct sunxi_de_be_reg {
+	u8 res0[0x800];			/* 0x000 */
+	u32 mode;			/* 0x800 */
+	u32 backcolor;			/* 0x804 */
+	u32 disp_size;			/* 0x808 */
+	u8 res1[0x4];			/* 0x80c */
+	u32 layer0_size;		/* 0x810 */
+	u32 layer1_size;		/* 0x814 */
+	u32 layer2_size;		/* 0x818 */
+	u32 layer3_size;		/* 0x81c */
+	u32 layer0_pos;			/* 0x820 */
+	u32 layer1_pos;			/* 0x824 */
+	u32 layer2_pos;			/* 0x828 */
+	u32 layer3_pos;			/* 0x82c */
+	u8 res2[0x10];			/* 0x830 */
+	u32 layer0_stride;		/* 0x840 */
+	u32 layer1_stride;		/* 0x844 */
+	u32 layer2_stride;		/* 0x848 */
+	u32 layer3_stride;		/* 0x84c */
+	u32 layer0_addr_low32b;		/* 0x850 */
+	u32 layer1_addr_low32b;		/* 0x854 */
+	u32 layer2_addr_low32b;		/* 0x858 */
+	u32 layer3_addr_low32b;		/* 0x85c */
+	u32 layer0_addr_high4b;		/* 0x860 */
+	u32 layer1_addr_high4b;		/* 0x864 */
+	u32 layer2_addr_high4b;		/* 0x868 */
+	u32 layer3_addr_high4b;		/* 0x86c */
+	u32 reg_ctrl;			/* 0x870 */
+	u8 res3[0xc];			/* 0x874 */
+	u32 color_key_max;		/* 0x880 */
+	u32 color_key_min;		/* 0x884 */
+	u32 color_key_config;		/* 0x888 */
+	u8 res4[0x4];			/* 0x88c */
+	u32 layer0_attr0_ctrl;		/* 0x890 */
+	u32 layer1_attr0_ctrl;		/* 0x894 */
+	u32 layer2_attr0_ctrl;		/* 0x898 */
+	u32 layer3_attr0_ctrl;		/* 0x89c */
+	u32 layer0_attr1_ctrl;		/* 0x8a0 */
+	u32 layer1_attr1_ctrl;		/* 0x8a4 */
+	u32 layer2_attr1_ctrl;		/* 0x8a8 */
+	u32 layer3_attr1_ctrl;		/* 0x8ac */
+};
+
+struct sunxi_lcdc_reg {
+	u32 ctrl;			/* 0x00 */
+	u32 int0;			/* 0x04 */
+	u32 int1;			/* 0x08 */
+	u8 res0[0x04];			/* 0x0c */
+	u32 frame_ctrl;			/* 0x10 */
+	u8 res1[0x2c];			/* 0x14 */
+	u32 tcon0_ctrl;			/* 0x40 */
+	u32 tcon0_dclk;			/* 0x44 */
+	u32 tcon0_basic_timing0;	/* 0x48 */
+	u32 tcon0_basic_timing1;	/* 0x4c */
+	u32 tcon0_basic_timing2;	/* 0x50 */
+	u32 tcon0_basic_timing3;	/* 0x54 */
+	u32 tcon0_hv_intf;		/* 0x58 */
+	u8 res2[0x04];			/* 0x5c */
+	u32 tcon0_cpu_intf;		/* 0x60 */
+	u32 tcon0_cpu_wr_dat;		/* 0x64 */
+	u32 tcon0_cpu_rd_dat0;		/* 0x68 */
+	u32 tcon0_cpu_rd_dat1;		/* 0x6c */
+	u32 tcon0_ttl_timing0;		/* 0x70 */
+	u32 tcon0_ttl_timing1;		/* 0x74 */
+	u32 tcon0_ttl_timing2;		/* 0x78 */
+	u32 tcon0_ttl_timing3;		/* 0x7c */
+	u32 tcon0_ttl_timing4;		/* 0x80 */
+	u32 tcon0_lvds_intf;		/* 0x84 */
+	u32 tcon0_io_polarity;		/* 0x88 */
+	u32 tcon0_io_tristate;		/* 0x8c */
+	u32 tcon1_ctrl;			/* 0x90 */
+	u32 tcon1_timing_source;	/* 0x94 */
+	u32 tcon1_timing_scale;		/* 0x98 */
+	u32 tcon1_timing_out;		/* 0x9c */
+	u32 tcon1_timing_h;		/* 0xa0 */
+	u32 tcon1_timing_v;		/* 0xa4 */
+	u32 tcon1_timing_sync;		/* 0xa8 */
+	u8 res3[0x44];			/* 0xac */
+	u32 tcon1_io_polarity;		/* 0xf0 */
+	u32 tcon1_io_tristate;		/* 0xf4 */
+};
+
+#define SUNXI_HDMI_CTRL			0x004
+#define SUNXI_HDMI_INT_CTRL		0x008
+#define SUNXI_HDMI_HPD			0x00c
+#define SUNXI_HDMI_VIDEO_CTRL		0x010
+#define SUNXI_HDMI_VIDEO_SIZE		0x014
+#define SUNXI_HDMI_VIDEO_BP		0x018
+#define SUNXI_HDMI_VIDEO_FP		0x01c
+#define SUNXI_HDMI_VIDEO_SPW		0x020
+#define SUNXI_HDMI_VIDEO_POLARITY	0x024
+#define SUNXI_HDMI_TX_DRIVER0		0x200
+#define SUNXI_HDMI_TX_DRIVER1		0x204
+#define SUNXI_HDMI_TX_DRIVER2		0x208
+#define SUNXI_HDMI_TX_DRIVER3		0x20C
+struct sunxi_hdmi_reg {
+	u32 version_id;			/* 0x000 */
+	u32 ctrl;			/* 0x004 */
+	u32 irq;			/* 0x008 */
+	u32 hpd;			/* 0x00c */
+	u32 video_ctrl;			/* 0x010 */
+	u32 video_size;			/* 0x014 */
+	u32 video_bp;			/* 0x018 */
+	u32 video_fp;			/* 0x01c */
+	u32 video_spw;			/* 0x020 */
+	u32 video_polarity;		/* 0x024 */
+	u8 res0[0x1d8];			/* 0x028 */
+	u32 pad_ctrl0;			/* 0x200 */
+	u32 pad_ctrl1;			/* 0x204 */
+	u32 pll_ctrl;			/* 0x208 */
+	u32 pll_dbg0;			/* 0x20c */
+};
+
+/*
+ * DE-BE register constants.
+ */
+#define SUNXI_DE_BE_WIDTH(x)			(((x) - 1) << 0)
+#define SUNXI_DE_BE_HEIGHT(y)			(((y) - 1) << 16)
+#define SUNXI_DE_BE_MODE_ENABLE			(1 << 0)
+#define SUNXI_DE_BE_MODE_START			(1 << 1)
+#define SUNXI_DE_BE_MODE_LAYER0_ENABLE		(1 << 8)
+#define SUNXI_DE_BE_LAYER_STRIDE(x)		((x) << 5)
+#define SUNXI_DE_BE_REG_CTRL_LOAD_REGS		(1 << 0)
+#define SUNXI_DE_BE_LAYER_ATTR1_FMT_XRGB8888	(0x09 << 8)
+
+/*
+ * LCDC register constants.
+ */
+#define SUNXI_LCDC_X(x)				(((x) - 1) << 16)
+#define SUNXI_LCDC_Y(y)				(((y) - 1) << 0)
+#define SUNXI_LCDC_CTRL_IO_MAP_MASK		(1 << 0)
+#define SUNXI_LCDC_CTRL_IO_MAP_TCON0		(0 << 0)
+#define SUNXI_LCDC_CTRL_IO_MAP_TCON1		(1 << 0)
+#define SUNXI_LCDC_CTRL_TCON_ENABLE		(1 << 31)
+#define SUNXI_LCDC_TCON0_DCLK_ENABLE		(0xf << 28)
+#define SUNXI_LCDC_TCON1_CTRL_CLK_DELAY(n)	(((n) & 0x1f) << 4)
+#define SUNXI_LCDC_TCON1_CTRL_ENABLE		(1 << 31)
+#define SUNXI_LCDC_TCON1_TIMING_H_BP(n)		(((n) - 1) << 0)
+#define SUNXI_LCDC_TCON1_TIMING_H_TOTAL(n)	(((n) - 1) << 16)
+#define SUNXI_LCDC_TCON1_TIMING_V_BP(n)		(((n) - 1) << 0)
+#define SUNXI_LCDC_TCON1_TIMING_V_TOTAL(n)	(((n) * 2) << 16)
+
+/*
+ * HDMI register constants.
+ */
+#define SUNXI_HDMI_X(x)				(((x) - 1) << 0)
+#define SUNXI_HDMI_Y(y)				(((y) - 1) << 16)
+#define SUNXI_HDMI_CTRL_ENABLE			(1 << 31)
+#define SUNXI_HDMI_IRQ_STATUS_FIFO_UF		(1 << 0)
+#define SUNXI_HDMI_IRQ_STATUS_FIFO_OF		(1 << 1)
+#define SUNXI_HDMI_IRQ_STATUS_BITS		0x73
+#define SUNXI_HDMI_HPD_DETECT			(1 << 0)
+#define SUNXI_HDMI_VIDEO_CTRL_ENABLE		(1 << 31)
+#define SUNXI_HDMI_VIDEO_POL_HOR		(1 << 0)
+#define SUNXI_HDMI_VIDEO_POL_VER		(1 << 1)
+#define SUNXI_HDMI_VIDEO_POL_TX_CLK		(0x3e0 << 16)
+
+#ifdef CONFIG_MACH_SUN6I
+#define SUNXI_HDMI_PAD_CTRL0_HDP		0x7e80000f
+#define SUNXI_HDMI_PAD_CTRL0_RUN		0x7e8000ff
+#else
+#define SUNXI_HDMI_PAD_CTRL0_HDP		0xfe800000
+#define SUNXI_HDMI_PAD_CTRL0_RUN		0xfe800000
+#endif
+
+#ifdef CONFIG_MACH_SUN4I
+#define SUNXI_HDMI_PAD_CTRL1			0x00d8c820
+#elif defined CONFIG_MACH_SUN6I
+#define SUNXI_HDMI_PAD_CTRL1			0x01ded030
+#else
+#define SUNXI_HDMI_PAD_CTRL1			0x00d8c830
+#endif
+#define SUNXI_HDMI_PAD_CTRL1_HALVE		(1 << 6)
+
+#ifdef CONFIG_MACH_SUN6I
+#define SUNXI_HDMI_PLL_CTRL			0xba48a308
+#define SUNXI_HDMI_PLL_CTRL_DIV(n)		(((n) - 1) << 4)
+#else
+#define SUNXI_HDMI_PLL_CTRL			0xfa4ef708
+#define SUNXI_HDMI_PLL_CTRL_DIV(n)		((n) << 4)
+#endif
+#define SUNXI_HDMI_PLL_CTRL_DIV_MASK		(0xf << 4)
+
+#define SUNXI_HDMI_PLL_DBG0_PLL3		(0 << 21)
+#define SUNXI_HDMI_PLL_DBG0_PLL7		(1 << 21)
+
+#endif /* _SUNXI_DISPLAY_H */
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index 5ea400e..422033a 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -216,4 +216,11 @@ config USB2_VBUS_PIN
 	---help---
 	See USB1_VBUS_PIN help text.
 
+config VIDEO
+	boolean "Enable graphical uboot console on HDMI"
+	default y
+	---help---
+	Say Y here to add support for using a cfb console on the HDMI output
+	found on most sunxi devices.
+
 endif
diff --git a/configs/Ippo_q8h_v5_defconfig b/configs/Ippo_q8h_v5_defconfig
index fc67bd9..53df213 100644
--- a/configs/Ippo_q8h_v5_defconfig
+++ b/configs/Ippo_q8h_v5_defconfig
@@ -4,3 +4,4 @@ CONFIG_ARCH_SUNXI=y
 CONFIG_MACH_SUN8I=y
 CONFIG_TARGET_IPPO_Q8H_V5=y
 CONFIG_DEFAULT_DEVICE_TREE="sun8i-a23-ippo-q8h-v5.dtb"
+CONFIG_VIDEO=n
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 14a6781..7301be3 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_VIDEO_SANDBOX_SDL) += sandbox_sdl.o
 obj-$(CONFIG_VIDEO_SED13806) += sed13806.o
 obj-$(CONFIG_VIDEO_SM501) += sm501.o
 obj-$(CONFIG_VIDEO_SMI_LYNXEM) += smiLynxEM.o videomodes.o
+obj-$(CONFIG_VIDEO_SUNXI) += sunxi_display.o
 obj-$(CONFIG_VIDEO_TEGRA) += tegra.o
 obj-$(CONFIG_VIDEO_VCXK) += bus_vcxk.o
 obj-$(CONFIG_FORMIKE) += formike.o
diff --git a/drivers/video/sunxi_display.c b/drivers/video/sunxi_display.c
new file mode 100644
index 0000000..74a92b5
--- /dev/null
+++ b/drivers/video/sunxi_display.c
@@ -0,0 +1,387 @@
+/*
+ * Display driver for Allwinner SoCs.
+ *
+ * (C) Copyright 2013-2014 Luc Verhaegen <libv@skynet.be>
+ * (C) Copyright 2014 Hans de Goede <hdegoede@redhat.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+
+#include <asm/arch/clock.h>
+#include <asm/arch/display.h>
+#include <asm/global_data.h>
+#include <asm/io.h>
+#include <linux/fb.h>
+#include <video_fb.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct sunxi_display {
+	GraphicDevice graphic_device;
+	bool enabled;
+} sunxi_display;
+
+static int
+sunxi_hdmi_hpd_detect(void)
+{
+	struct sunxi_ccm_reg * const ccm =
+		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+	struct sunxi_hdmi_reg * const hdmi =
+		(struct sunxi_hdmi_reg *)SUNXI_HDMI_BASE;
+
+	/* Set pll3 to 300MHz */
+	clock_set_pll3(300000000);
+
+	/* Set hdmi parent to pll3 */
+	clrsetbits_le32(&ccm->hdmi_clk_cfg, CCM_HDMI_CTRL_PLL_MASK,
+			CCM_HDMI_CTRL_PLL3);
+
+	/* Set ahb gating to pass */
+	setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_HDMI);
+
+	/* Clock on */
+	setbits_le32(&ccm->hdmi_clk_cfg, CCM_HDMI_CTRL_GATE);
+
+	writel(SUNXI_HDMI_CTRL_ENABLE, &hdmi->ctrl);
+	writel(SUNXI_HDMI_PAD_CTRL0_HDP, &hdmi->pad_ctrl0);
+
+	udelay(1000);
+
+	if (readl(&hdmi->hpd) & SUNXI_HDMI_HPD_DETECT)
+		return 1;
+
+	/* No need to keep these running */
+	clrbits_le32(&hdmi->ctrl, SUNXI_HDMI_CTRL_ENABLE);
+	clrbits_le32(&ccm->hdmi_clk_cfg, CCM_HDMI_CTRL_GATE);
+	clrbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_HDMI);
+	clock_set_pll3(0);
+
+	return 0;
+}
+
+/*
+ * This is the entity that mixes and matches the different layers and inputs.
+ * Allwinner calls it the back-end, but i like composer better.
+ */
+static void
+sunxi_composer_init(void)
+{
+	struct sunxi_ccm_reg * const ccm =
+		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+	struct sunxi_de_be_reg * const de_be =
+		(struct sunxi_de_be_reg *)SUNXI_DE_BE0_BASE;
+	int i;
+
+	/* Clocks on */
+	setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_DE_BE0);
+	setbits_le32(&ccm->dram_clk_gate, 1 << CCM_DRAM_GATE_OFFSET_DE_BE0);
+	clock_set_de_mod_clock(&ccm->be0_clk_cfg, 300000000);
+
+	/* Engine bug, clear registers after reset */
+	for (i = 0x0800; i < 0x1000; i += 4)
+		writel(0, SUNXI_DE_BE0_BASE + i);
+
+	setbits_le32(&de_be->mode, SUNXI_DE_BE_MODE_ENABLE);
+}
+
+static void
+sunxi_composer_mode_set(struct fb_videomode *mode, unsigned int address)
+{
+	struct sunxi_de_be_reg * const de_be =
+		(struct sunxi_de_be_reg *)SUNXI_DE_BE0_BASE;
+
+	writel(SUNXI_DE_BE_HEIGHT(mode->yres) | SUNXI_DE_BE_WIDTH(mode->xres),
+	       &de_be->disp_size);
+	writel(SUNXI_DE_BE_HEIGHT(mode->yres) | SUNXI_DE_BE_WIDTH(mode->xres),
+	       &de_be->layer0_size);
+	writel(SUNXI_DE_BE_LAYER_STRIDE(mode->xres), &de_be->layer0_stride);
+	writel(address << 3, &de_be->layer0_addr_low32b);
+	writel(address >> 29, &de_be->layer0_addr_high4b);
+	writel(SUNXI_DE_BE_LAYER_ATTR1_FMT_XRGB8888, &de_be->layer0_attr1_ctrl);
+
+	setbits_le32(&de_be->mode, SUNXI_DE_BE_MODE_LAYER0_ENABLE);
+}
+
+/*
+ * LCDC, what allwinner calls a CRTC, so timing controller and serializer.
+ */
+static void
+sunxi_lcdc_pll_set(int dotclock, int *clk_div, int *clk_double)
+{
+	struct sunxi_ccm_reg * const ccm =
+		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+	int value, n, m, diff;
+	int best_n = 0, best_m = 0, best_diff = 0x0FFFFFFF;
+	int best_double = 0;
+
+	/*
+	 * Find the lowest divider resulting in a matching clock, if there
+	 * is no match, pick the closest lower clock, as monitors tend to
+	 * not sync to higher frequencies.
+	 */
+	for (m = 15; m > 0; m--) {
+		n = (m * dotclock) / 3000;
+
+		if ((n >= 9) && (n <= 127)) {
+			value = (3000 * n) / m;
+			diff = dotclock - value;
+			if (diff < best_diff) {
+				best_diff = diff;
+				best_m = m;
+				best_n = n;
+				best_double = 0;
+			}
+		}
+
+		/* These are just duplicates */
+		if (!(m & 1))
+			continue;
+
+		n = (m * dotclock) / 6000;
+		if ((n >= 9) && (n <= 127)) {
+			value = (6000 * n) / m;
+			diff = dotclock - value;
+			if (diff < best_diff) {
+				best_diff = diff;
+				best_m = m;
+				best_n = n;
+				best_double = 1;
+			}
+		}
+	}
+
+#if 1
+	printf("dotclock: %dkHz = %dkHz: (%d * 3MHz * %d) / %d\n",
+	       dotclock, (best_double + 1) * 3000 * best_n / best_m,
+	       best_double + 1, best_n, best_m);
+#endif
+
+	clock_set_pll3(best_n * 3000000);
+
+	writel(CCM_LCD_CH1_CTRL_GATE |
+	    (best_double ? CCM_LCD_CH1_CTRL_PLL3_2X : CCM_LCD_CH1_CTRL_PLL3) |
+	    CCM_LCD_CH1_CTRL_M(best_m), &ccm->lcd0_ch1_clk_cfg);
+
+	*clk_div = best_m;
+	*clk_double = best_double;
+}
+
+static void
+sunxi_lcdc_init(void)
+{
+	struct sunxi_ccm_reg * const ccm =
+		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+	struct sunxi_lcdc_reg * const lcdc =
+		(struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
+
+	/* Reset off */
+	setbits_le32(&ccm->lcd0_ch0_clk_cfg, CCM_LCD_CH0_CTRL_RST);
+
+	/* Clock on */
+	setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_LCD0);
+
+	/* Init lcdc */
+	writel(0, &lcdc->ctrl); /* Disable tcon */
+	writel(0, &lcdc->int0); /* Disable all interrupts */
+
+	/* Disable tcon0 dot clock */
+	clrbits_le32(&lcdc->tcon0_dclk, SUNXI_LCDC_TCON0_DCLK_ENABLE);
+
+	/* Set all io lines to tristate */
+	writel(0xffffffff, &lcdc->tcon0_io_tristate);
+	writel(0xffffffff, &lcdc->tcon1_io_tristate);
+}
+
+static void
+sunxi_lcdc_mode_set(struct fb_videomode *mode, int *clk_div, int *clk_double)
+{
+	struct sunxi_lcdc_reg * const lcdc =
+		(struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
+	int bp, total;
+
+	/* Use tcon1 */
+	clrsetbits_le32(&lcdc->ctrl, SUNXI_LCDC_CTRL_IO_MAP_MASK,
+			SUNXI_LCDC_CTRL_IO_MAP_TCON1);
+
+	/* Enabled, 0x1e start delay */
+	writel(SUNXI_LCDC_TCON1_CTRL_ENABLE |
+	       SUNXI_LCDC_TCON1_CTRL_CLK_DELAY(0x1e), &lcdc->tcon1_ctrl);
+
+	writel(SUNXI_LCDC_X(mode->xres) | SUNXI_LCDC_Y(mode->yres),
+	       &lcdc->tcon1_timing_source);
+	writel(SUNXI_LCDC_X(mode->xres) | SUNXI_LCDC_Y(mode->yres),
+	       &lcdc->tcon1_timing_scale);
+	writel(SUNXI_LCDC_X(mode->xres) | SUNXI_LCDC_Y(mode->yres),
+	       &lcdc->tcon1_timing_out);
+
+	bp = mode->hsync_len + mode->left_margin;
+	total = mode->xres + mode->right_margin + bp;
+	writel(SUNXI_LCDC_TCON1_TIMING_H_TOTAL(total) |
+	       SUNXI_LCDC_TCON1_TIMING_H_BP(bp), &lcdc->tcon1_timing_h);
+
+	bp = mode->vsync_len + mode->upper_margin;
+	total = mode->yres + mode->lower_margin + bp;
+	writel(SUNXI_LCDC_TCON1_TIMING_V_TOTAL(total) |
+	       SUNXI_LCDC_TCON1_TIMING_V_BP(bp), &lcdc->tcon1_timing_v);
+
+	writel(SUNXI_LCDC_X(mode->hsync_len) | SUNXI_LCDC_Y(mode->vsync_len),
+	       &lcdc->tcon1_timing_sync);
+
+	sunxi_lcdc_pll_set(mode->pixclock, clk_div, clk_double);
+}
+
+static void
+sunxi_hdmi_mode_set(struct fb_videomode *mode, int clk_div, int clk_double)
+{
+	struct sunxi_hdmi_reg * const hdmi =
+		(struct sunxi_hdmi_reg *)SUNXI_HDMI_BASE;
+	int x, y;
+
+	/* Write clear interrupt status bits */
+	writel(SUNXI_HDMI_IRQ_STATUS_BITS, &hdmi->irq);
+
+	/* Init various registers, select pll3 as clock source */
+	writel(SUNXI_HDMI_VIDEO_POL_TX_CLK, &hdmi->video_polarity);
+	writel(SUNXI_HDMI_PAD_CTRL0_RUN, &hdmi->pad_ctrl0);
+	writel(SUNXI_HDMI_PAD_CTRL1, &hdmi->pad_ctrl1);
+	writel(SUNXI_HDMI_PLL_CTRL, &hdmi->pll_ctrl);
+	writel(SUNXI_HDMI_PLL_DBG0_PLL3, &hdmi->pll_dbg0);
+
+	/* Setup clk div and doubler */
+	clrsetbits_le32(&hdmi->pll_ctrl, SUNXI_HDMI_PLL_CTRL_DIV_MASK,
+			SUNXI_HDMI_PLL_CTRL_DIV(clk_div));
+	if (!clk_double)
+		setbits_le32(&hdmi->pad_ctrl1, SUNXI_HDMI_PAD_CTRL1_HALVE);
+
+	/* Setup timing registers */
+	writel(SUNXI_HDMI_Y(mode->yres) | SUNXI_HDMI_X(mode->xres),
+	       &hdmi->video_size);
+
+	x = mode->hsync_len + mode->left_margin;
+	y = mode->vsync_len + mode->upper_margin;
+	writel(SUNXI_HDMI_Y(y) | SUNXI_HDMI_X(x), &hdmi->video_bp);
+
+	x = mode->right_margin;
+	y = mode->lower_margin;
+	writel(SUNXI_HDMI_Y(y) | SUNXI_HDMI_X(x), &hdmi->video_fp);
+
+	x = mode->hsync_len;
+	y = mode->vsync_len;
+	writel(SUNXI_HDMI_Y(y) | SUNXI_HDMI_X(x), &hdmi->video_spw);
+
+	if (mode->sync & FB_SYNC_HOR_HIGH_ACT)
+		setbits_le32(&hdmi->video_polarity, SUNXI_HDMI_VIDEO_POL_HOR);
+
+	if (mode->sync & FB_SYNC_VERT_HIGH_ACT)
+		setbits_le32(&hdmi->video_polarity, SUNXI_HDMI_VIDEO_POL_VER);
+}
+
+static void
+sunxi_engines_init(void)
+{
+	sunxi_composer_init();
+	sunxi_lcdc_init();
+}
+
+static void
+sunxi_mode_set(struct fb_videomode *mode, unsigned int address)
+{
+	struct sunxi_de_be_reg * const de_be =
+		(struct sunxi_de_be_reg *)SUNXI_DE_BE0_BASE;
+	struct sunxi_lcdc_reg * const lcdc =
+		(struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
+	struct sunxi_hdmi_reg * const hdmi =
+		(struct sunxi_hdmi_reg *)SUNXI_HDMI_BASE;
+	int clk_div, clk_double;
+	int retries = 3;
+
+retry:
+	clrbits_le32(&hdmi->video_ctrl, SUNXI_HDMI_VIDEO_CTRL_ENABLE);
+	clrbits_le32(&lcdc->ctrl, SUNXI_LCDC_CTRL_TCON_ENABLE);
+	clrbits_le32(&de_be->mode, SUNXI_DE_BE_MODE_START);
+
+	sunxi_composer_mode_set(mode, address);
+	sunxi_lcdc_mode_set(mode, &clk_div, &clk_double);
+	sunxi_hdmi_mode_set(mode, clk_div, clk_double);
+
+	setbits_le32(&de_be->reg_ctrl, SUNXI_DE_BE_REG_CTRL_LOAD_REGS);
+	setbits_le32(&de_be->mode, SUNXI_DE_BE_MODE_START);
+
+	udelay(1000000 / mode->refresh + 500);
+
+	setbits_le32(&lcdc->ctrl, SUNXI_LCDC_CTRL_TCON_ENABLE);
+
+	udelay(1000000 / mode->refresh + 500);
+
+	setbits_le32(&hdmi->video_ctrl, SUNXI_HDMI_VIDEO_CTRL_ENABLE);
+
+	udelay(1000000 / mode->refresh + 500);
+
+	/* Sometimes the display pipeline does not sync up properly, if
+	   this happens the hdmi fifo underrun or overrun bits are set */
+	if (readl(&hdmi->irq) &
+	    (SUNXI_HDMI_IRQ_STATUS_FIFO_UF | SUNXI_HDMI_IRQ_STATUS_FIFO_OF)) {
+		if (retries--)
+			goto retry;
+		eprintf("HDMI fifo under or overrun\n");
+	}
+}
+
+void *
+video_hw_init(void)
+{
+	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
+	/*
+	 * Vesa standard 1024x768 at 60
+	 * 65.0  1024 1032 1176 1344  768 771 777 806  -hsync -vsync
+	 */
+	struct fb_videomode mode = {
+		.name = "1024x768",
+		.refresh = 60,
+		.xres = 1024,
+		.yres = 768,
+		.pixclock = 65000,
+		.left_margin = 160,
+		.right_margin = 24,
+		.upper_margin = 29,
+		.lower_margin = 3,
+		.hsync_len = 136,
+		.vsync_len = 6,
+		.sync = 0,
+		.vmode = 0,
+		.flag = 0,
+	};
+	int ret;
+
+	memset(&sunxi_display, 0, sizeof(struct sunxi_display));
+
+	printf("Reserved %dkB of RAM for Framebuffer.\n",
+	       CONFIG_SUNXI_FB_SIZE >> 10);
+	gd->fb_base = gd->ram_top;
+
+	ret = sunxi_hdmi_hpd_detect();
+	if (!ret)
+		return NULL;
+
+	printf("HDMI connected.\n");
+	sunxi_display.enabled = true;
+
+	printf("Setting up a %s console.\n", mode.name);
+	sunxi_engines_init();
+	sunxi_mode_set(&mode, gd->fb_base - CONFIG_SYS_SDRAM_BASE);
+
+	/*
+	 * These are the only members of this structure that are used. All the
+	 * others are driver specific. There is nothing to decribe pitch or
+	 * stride, but we are lucky with our hw.
+	 */
+	graphic_device->frameAdrs = gd->fb_base;
+	graphic_device->gdfIndex = GDF_32BIT_X888RGB;
+	graphic_device->gdfBytesPP = 4;
+	graphic_device->winSizeX = mode.xres;
+	graphic_device->winSizeY = mode.yres;
+
+	return graphic_device;
+}
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index ce038ed..532fdb7 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -197,6 +197,30 @@
 #define CONFIG_SPL_GPIO_SUPPORT
 #define CONFIG_CMD_GPIO
 
+#ifdef CONFIG_VIDEO
+/*
+ * The amount of RAM that is reserved for the FB. This will not show up as
+ * RAM to the kernel, but will be reclaimed by a KMS driver in future.
+ */
+#define CONFIG_SUNXI_FB_SIZE (8 << 20)
+
+#define CONFIG_VIDEO_SUNXI
+
+#define CONFIG_CFB_CONSOLE
+#define CONFIG_VIDEO_SW_CURSOR
+#define CONFIG_VIDEO_LOGO
+
+/* allow both serial and cfb console. */
+#define CONFIG_CONSOLE_MUX
+/* this is needed so the above will actually do something */
+#define CONFIG_SYS_CONSOLE_IS_IN_ENV
+/* stop x86 thinking in cfbconsole from trying to init a pc keyboard */
+#define CONFIG_VGA_AS_SINGLE_DEVICE
+
+#define CONFIG_SYS_MEM_TOP_HIDE ((CONFIG_SUNXI_FB_SIZE + 0xFFF) & ~0xFFF)
+
+#endif /* CONFIG_VIDEO */
+
 /* Ethernet support */
 #ifdef CONFIG_SUNXI_EMAC
 #define CONFIG_MII			/* MII PHY management		*/
@@ -266,7 +290,17 @@
 
 #include <config_distro_bootcmd.h>
 
+#ifdef CONFIG_VIDEO
+#define CONSOLE_ENV_SETTINGS \
+	"stdin=serial\0" \
+	"stdout=serial,vga\0" \
+	"stderr=serial,vga\0"
+#else
+#define CONSOLE_ENV_SETTINGS
+#endif
+
 #define CONFIG_EXTRA_ENV_SETTINGS \
+	CONSOLE_ENV_SETTINGS \
 	MEM_LAYOUT_ENV_SETTINGS \
 	"fdtfile=" CONFIG_FDTFILE "\0" \
 	"console=ttyS0,115200\0" \
-- 
2.1.0

[U-Boot] [PATCH 4/6] sunxi: video: Add sun6i support

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/sunxi_display.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/video/sunxi_display.c b/drivers/video/sunxi_display.c
index 74a92b5..1058771 100644
--- a/drivers/video/sunxi_display.c
+++ b/drivers/video/sunxi_display.c
@@ -39,6 +39,9 @@ sunxi_hdmi_hpd_detect(void)
 			CCM_HDMI_CTRL_PLL3);
 
 	/* Set ahb gating to pass */
+#ifdef CONFIG_MACH_SUN6I
+	setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_HDMI);
+#endif
 	setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_HDMI);
 
 	/* Clock on */
@@ -56,6 +59,9 @@ sunxi_hdmi_hpd_detect(void)
 	clrbits_le32(&hdmi->ctrl, SUNXI_HDMI_CTRL_ENABLE);
 	clrbits_le32(&ccm->hdmi_clk_cfg, CCM_HDMI_CTRL_GATE);
 	clrbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_HDMI);
+#ifdef CONFIG_MACH_SUN6I
+	clrbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_HDMI);
+#endif
 	clock_set_pll3(0);
 
 	return 0;
@@ -74,6 +80,11 @@ sunxi_composer_init(void)
 		(struct sunxi_de_be_reg *)SUNXI_DE_BE0_BASE;
 	int i;
 
+#ifdef CONFIG_MACH_SUN6I
+	/* Reset off */
+	setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_DE_BE0);
+#endif
+
 	/* Clocks on */
 	setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_DE_BE0);
 	setbits_le32(&ccm->dram_clk_gate, 1 << CCM_DRAM_GATE_OFFSET_DE_BE0);
@@ -177,7 +188,11 @@ sunxi_lcdc_init(void)
 		(struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
 
 	/* Reset off */
+#ifdef CONFIG_MACH_SUN6I
+	setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_LCD0);
+#else
 	setbits_le32(&ccm->lcd0_ch0_clk_cfg, CCM_LCD_CH0_CTRL_RST);
+#endif
 
 	/* Clock on */
 	setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_LCD0);
@@ -232,6 +247,19 @@ sunxi_lcdc_mode_set(struct fb_videomode *mode, int *clk_div, int *clk_double)
 	sunxi_lcdc_pll_set(mode->pixclock, clk_div, clk_double);
 }
 
+#ifdef CONFIG_MACH_SUN6I
+static void
+sunxi_drc_init(void)
+{
+	struct sunxi_ccm_reg * const ccm =
+		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+	/* On sun6i the drc must be clocked even when in pass-through mode */
+	setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_DRC0);
+	clock_set_de_mod_clock(&ccm->iep_drc0_clk_cfg, 300000000);
+}
+#endif
+
 static void
 sunxi_hdmi_mode_set(struct fb_videomode *mode, int clk_div, int clk_double)
 {
@@ -283,6 +311,9 @@ sunxi_engines_init(void)
 {
 	sunxi_composer_init();
 	sunxi_lcdc_init();
+#ifdef CONFIG_MACH_SUN6I
+	sunxi_drc_init();
+#endif
 }
 
 static void
-- 
2.1.0

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Hans de Goede]

From: Luc Verhaegen <libv@skynet.be>

Add simplefb support, note this depends on the kernel having support for
the clocks property which has recently been added to the simplefb devicetree
binding.

Signed-off-by: Luc Verhaegen <libv@skynet.be>
[hdegoede at redhat.com: Use pre-populated simplefb node under /chosen as
 disussed on the devicetree list]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/include/asm/arch-sunxi/display.h |  4 ++
 board/sunxi/board.c                       | 11 ++++++
 drivers/video/sunxi_display.c             | 66 +++++++++++++++++++++++++++++++
 include/configs/sunxi-common.h            |  8 ++++
 4 files changed, 89 insertions(+)

diff --git a/arch/arm/include/asm/arch-sunxi/display.h b/arch/arm/include/asm/arch-sunxi/display.h
index 8ac7d1b..e38a401 100644
--- a/arch/arm/include/asm/arch-sunxi/display.h
+++ b/arch/arm/include/asm/arch-sunxi/display.h
@@ -195,4 +195,8 @@ struct sunxi_hdmi_reg {
 #define SUNXI_HDMI_PLL_DBG0_PLL3		(0 << 21)
 #define SUNXI_HDMI_PLL_DBG0_PLL7		(1 << 21)
 
+#ifdef CONFIG_VIDEO_DT_SIMPLEFB
+void sunxi_simplefb_setup(void *blob);
+#endif
+
 #endif /* _SUNXI_DISPLAY_H */
diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index e6ec5b8..d4530e8 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -24,6 +24,7 @@
 #endif
 #include <asm/arch/clock.h>
 #include <asm/arch/cpu.h>
+#include <asm/arch/display.h>
 #include <asm/arch/dram.h>
 #include <asm/arch/gpio.h>
 #include <asm/arch/mmc.h>
@@ -237,3 +238,13 @@ int misc_init_r(void)
 	return 0;
 }
 #endif
+
+#ifdef CONFIG_OF_BOARD_SETUP
+void
+ft_board_setup(void *blob, bd_t *bd)
+{
+#ifdef CONFIG_VIDEO_DT_SIMPLEFB
+	sunxi_simplefb_setup(blob);
+#endif
+}
+#endif /* CONFIG_OF_BOARD_SETUP */
diff --git a/drivers/video/sunxi_display.c b/drivers/video/sunxi_display.c
index 1058771..a47f575 100644
--- a/drivers/video/sunxi_display.c
+++ b/drivers/video/sunxi_display.c
@@ -13,6 +13,7 @@
 #include <asm/arch/display.h>
 #include <asm/global_data.h>
 #include <asm/io.h>
+#include <fdtdec.h>
 #include <linux/fb.h>
 #include <video_fb.h>
 
@@ -416,3 +417,68 @@ video_hw_init(void)
 
 	return graphic_device;
 }
+
+/*
+ * Simplefb support.
+ */
+#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
+void
+sunxi_simplefb_setup(void *blob)
+{
+	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
+	const char *node = "/chosen/framebuffer0-hdmi";
+	const char *format = "x8r8g8b8";
+	const char *okay = "okay";
+	char name[32];
+	fdt32_t cells[2];
+	int offset, stride, ret;
+
+	if (!sunxi_display.enabled)
+		return;
+
+	offset = fdt_path_offset(blob, node);
+	if (offset < 0) {
+		eprintf("Cannot setup simplefb: %s node not found\n", node);
+		return;
+	}
+
+	snprintf(name, sizeof(name), "framebuffer@%08lx", gd->fb_base);
+	ret = fdt_set_name(blob, offset, name);
+	if (ret < 0)
+		goto error;
+
+	cells[0] = cpu_to_fdt32(gd->fb_base);
+	cells[1] = cpu_to_fdt32(CONFIG_SUNXI_FB_SIZE);
+	ret = fdt_setprop(blob, offset, "reg", cells, sizeof(cells[0]) * 2);
+	if (ret < 0)
+		goto error;
+
+	cells[0] = cpu_to_fdt32(graphic_device->winSizeX);
+	ret = fdt_setprop(blob, offset, "width", cells, sizeof(cells[0]));
+	if (ret < 0)
+		goto error;
+
+	cells[0] = cpu_to_fdt32(graphic_device->winSizeY);
+	ret = fdt_setprop(blob, offset, "height", cells, sizeof(cells[0]));
+	if (ret < 0)
+		goto error;
+
+	stride = graphic_device->winSizeX * graphic_device->gdfBytesPP;
+	cells[0] = cpu_to_fdt32(stride);
+	ret = fdt_setprop(blob, offset, "stride", cells, sizeof(cells[0]));
+	if (ret < 0)
+		goto error;
+
+	ret = fdt_setprop(blob, offset, "format", format, strlen(format) + 1);
+	if (ret < 0)
+		goto error;
+
+	ret = fdt_setprop(blob, offset, "status", okay, strlen(okay) + 1);
+	if (ret < 0)
+		goto error;
+
+	return;
+error:
+	eprintf("Cannot setup simplefb: Error setting properties\n");
+}
+#endif /* CONFIG_OF_BOARD_SETUP && CONFIG_VIDEO_DT_SIMPLEFB */
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 532fdb7..d7d8571 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -204,6 +204,9 @@
  */
 #define CONFIG_SUNXI_FB_SIZE (8 << 20)
 
+/* Do we want to initialize a simple FB? */
+#define CONFIG_VIDEO_DT_SIMPLEFB
+
 #define CONFIG_VIDEO_SUNXI
 
 #define CONFIG_CFB_CONSOLE
@@ -219,6 +222,11 @@
 
 #define CONFIG_SYS_MEM_TOP_HIDE ((CONFIG_SUNXI_FB_SIZE + 0xFFF) & ~0xFFF)
 
+/* To be able to hook simplefb into dt */
+#ifdef CONFIG_VIDEO_DT_SIMPLEFB
+#define CONFIG_OF_BOARD_SETUP
+#endif
+
 #endif /* CONFIG_VIDEO */
 
 /* Ethernet support */
-- 
2.1.0

[U-Boot] [PATCH 6/6] sunxi: Add usb keyboard Kconfig option

[Hans de Goede]

For use together with the hdmi console.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 board/sunxi/Kconfig            |  7 +++++++
 configs/Ippo_q8h_v5_defconfig  |  1 +
 include/configs/sunxi-common.h | 25 +++++++++++++++++++++----
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index 422033a..246cd9a 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -223,4 +223,11 @@ config VIDEO
 	Say Y here to add support for using a cfb console on the HDMI output
 	found on most sunxi devices.
 
+config USB_KEYBOARD
+	boolean "Enable USB keyboard support"
+	default y
+	---help---
+	Say Y here to add support for using a USB keyboard (typically used
+	in combination with a graphical console on HDMI).
+
 endif
diff --git a/configs/Ippo_q8h_v5_defconfig b/configs/Ippo_q8h_v5_defconfig
index 53df213..50c2f93 100644
--- a/configs/Ippo_q8h_v5_defconfig
+++ b/configs/Ippo_q8h_v5_defconfig
@@ -5,3 +5,4 @@ CONFIG_MACH_SUN8I=y
 CONFIG_TARGET_IPPO_Q8H_V5=y
 CONFIG_DEFAULT_DEVICE_TREE="sun8i-a23-ippo-q8h-v5.dtb"
 CONFIG_VIDEO=n
+CONFIG_USB_KEYBOARD=n
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index d7d8571..5d1b611 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -249,6 +249,12 @@
 #define CONFIG_USB_STORAGE
 #endif
 
+#ifdef CONFIG_USB_KEYBOARD
+#define CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
+#define CONFIG_PREBOOT
+#define CONFIG_SYS_STDIO_DEREGISTER
+#endif
+
 #if !defined CONFIG_ENV_IS_IN_MMC && \
     !defined CONFIG_ENV_IS_IN_NAND && \
     !defined CONFIG_ENV_IS_IN_FAT && \
@@ -298,17 +304,28 @@
 
 #include <config_distro_bootcmd.h>
 
+#ifdef CONFIG_USB_KEYBOARD
+#define CONSOLE_IN_SETTINGS \
+	"preboot=usb start\0" \
+	"stdin=serial,usbkbd\0"
+#else
+#define CONSOLE_IN_SETTINGS \
+	"stdin=serial\0"
+#endif
+
 #ifdef CONFIG_VIDEO
-#define CONSOLE_ENV_SETTINGS \
-	"stdin=serial\0" \
+#define CONSOLE_OUT_SETTINGS \
 	"stdout=serial,vga\0" \
 	"stderr=serial,vga\0"
 #else
-#define CONSOLE_ENV_SETTINGS
+#define CONSOLE_OUT_SETTINGS \
+	"stdout=serial\0" \
+	"stderr=serial\0"
 #endif
 
 #define CONFIG_EXTRA_ENV_SETTINGS \
-	CONSOLE_ENV_SETTINGS \
+	CONSOLE_IN_SETTINGS \
+	CONSOLE_OUT_SETTINGS \
 	MEM_LAYOUT_ENV_SETTINGS \
 	"fdtfile=" CONFIG_FDTFILE "\0" \
 	"console=ttyS0,115200\0" \
-- 
2.1.0

[U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi

[Anatolij Gustschin]

Hi Hans,

this patch looks good, only a few minor comments below.

On Fri, 14 Nov 2014 17:54:45 +0100
Hans de Goede <hdegoede@redhat.com> wrote:
...
> diff --git a/arch/arm/include/asm/arch-sunxi/display.h b/arch/arm/include/asm/arch-sunxi/display.h
> new file mode 100644
> index 0000000..8ac7d1b
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-sunxi/display.h
> @@ -0,0 +1,198 @@
> +/*
> + * Sunxi platform display cntroller register and constant defines

s/cntroller/controller/

not need to resubmit the patch, this can be fixed when applying.

...
> +#if 1
> +	printf("dotclock: %dkHz = %dkHz: (%d * 3MHz * %d) / %d\n",
> +	       dotclock, (best_double + 1) * 3000 * best_n / best_m,
> +	       best_double + 1, best_n, best_m);
> +#endif

please drop #if 1 / #endif when applying the patch.
here, printf() could be replaced by debug() maybe ?
 
...
> +	udelay(1000000 / mode->refresh + 500);
> +
> +	/* Sometimes the display pipeline does not sync up properly, if
> +	   this happens the hdmi fifo underrun or overrun bits are set */

we use

/*
 * multi-line
 * comment
 */

style.

Thanks,

Anatolij

[U-Boot] [PATCH 4/6] sunxi: video: Add sun6i support

[Anatolij Gustschin]

On Fri, 14 Nov 2014 17:54:46 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/video/sunxi_display.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)

Acked-by: Anatolij Gustschin <agust@denx.de>

Thanks,

Anatolij

[U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi

[Anatolij Gustschin]

On Fri, 14 Nov 2014 21:17:39 +0100
Anatolij Gustschin <agust@denx.de> wrote:
...
> this patch looks good, only a few minor comments below.

I forgot to mention that with these comments addressed,
you can add

Acked-by: Anatolij Gustschin <agust@denx.de>

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Anatolij Gustschin]

On Fri, 14 Nov 2014 17:54:47 +0100
Hans de Goede <hdegoede@redhat.com> wrote:
...
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index 532fdb7..d7d8571 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -204,6 +204,9 @@
>   */
>  #define CONFIG_SUNXI_FB_SIZE (8 << 20)
>  
> +/* Do we want to initialize a simple FB? */
> +#define CONFIG_VIDEO_DT_SIMPLEFB
> +
>  #define CONFIG_VIDEO_SUNXI
>  
>  #define CONFIG_CFB_CONSOLE
> @@ -219,6 +222,11 @@
>  
>  #define CONFIG_SYS_MEM_TOP_HIDE ((CONFIG_SUNXI_FB_SIZE + 0xFFF) & ~0xFFF)
>  
> +/* To be able to hook simplefb into dt */
> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB
> +#define CONFIG_OF_BOARD_SETUP
> +#endif

defining CONFIG_OF_BOARD_SETUP should be independent of defining
CONFIG_VIDEO or CONFIG_VIDEO_DT_SIMPLEFB. DT fixup by board setup
code is often done i.e. for auto-detected memory size, adjusting
different node properties or adding/deleting whole nodes. If
someone defines CONFIG_OF_BOARD_SETUP for these purposes and also
defines CONFIG_VIDEO_DT_SIMPLEFB, the "already defined" warnings
will appear. This can be left as is for now, but should be addressed
in the long term.

Thanks,

Anatolij

[U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi

[Hans de Goede]

Hi,

On 11/14/2014 09:24 PM, Anatolij Gustschin wrote:
> On Fri, 14 Nov 2014 21:17:39 +0100
> Anatolij Gustschin <agust@denx.de> wrote:
> ...
>> this patch looks good, only a few minor comments below.
> 
> I forgot to mention that with these comments addressed,
> you can add

Thanks for the review!

I'll go and address your comments in my local tree.

> Acked-by: Anatolij Gustschin <agust@denx.de>

And then add your acked-by.

Regards,

Hans

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Hans de Goede]

Hi,

On 11/14/2014 11:22 PM, Anatolij Gustschin wrote:
> On Fri, 14 Nov 2014 17:54:47 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> ...
>> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
>> index 532fdb7..d7d8571 100644
>> --- a/include/configs/sunxi-common.h
>> +++ b/include/configs/sunxi-common.h
>> @@ -204,6 +204,9 @@
>>   */
>>  #define CONFIG_SUNXI_FB_SIZE (8 << 20)
>>  
>> +/* Do we want to initialize a simple FB? */
>> +#define CONFIG_VIDEO_DT_SIMPLEFB
>> +
>>  #define CONFIG_VIDEO_SUNXI
>>  
>>  #define CONFIG_CFB_CONSOLE
>> @@ -219,6 +222,11 @@
>>  
>>  #define CONFIG_SYS_MEM_TOP_HIDE ((CONFIG_SUNXI_FB_SIZE + 0xFFF) & ~0xFFF)
>>  
>> +/* To be able to hook simplefb into dt */
>> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB
>> +#define CONFIG_OF_BOARD_SETUP
>> +#endif
> 
> defining CONFIG_OF_BOARD_SETUP should be independent of defining
> CONFIG_VIDEO or CONFIG_VIDEO_DT_SIMPLEFB. DT fixup by board setup
> code is often done i.e. for auto-detected memory size, adjusting
> different node properties or adding/deleting whole nodes. If
> someone defines CONFIG_OF_BOARD_SETUP for these purposes and also
> defines CONFIG_VIDEO_DT_SIMPLEFB, the "already defined" warnings
> will appear. This can be left as is for now, but should be addressed
> in the long term.

Currently the only thing we need CONFIG_OF_BOARD_SETUP for on sunxi
is CONFIG_VIDEO_DT_SIMPLEFB, so for now this makes sense, but your 100%
right that long term we probably want to do this differently.

Regards,

Hans

[U-Boot] [PATCH 1/6] sun4i: Rename dram_clk_cfg to dram_clk_gate

[Ian Campbell]

On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> The data sheet just calls it DRAM_CLK_REG, and on sun6i we've both a
> dram_clk_cfg and dram_clk_gate, and the sun4i reg matches dram_clk_gate on
> sun6i, so name it the same on sun4i.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ian Campbell <ijc@hellion.org.uk>

> ---
>  arch/arm/cpu/armv7/sunxi/dram_sun4i.c         | 4 ++--
>  arch/arm/include/asm/arch-sunxi/clock_sun4i.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> index dc9fdb9..ec8aaa7 100644
> --- a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> @@ -428,9 +428,9 @@ static void dramc_clock_output_en(u32 on)
>  #ifdef CONFIG_MACH_SUN4I
>  	struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
>  	if (on)
> -		setbits_le32(&ccm->dram_clk_cfg, CCM_DRAM_CTRL_DCLK_OUT);
> +		setbits_le32(&ccm->dram_clk_gate, CCM_DRAM_CTRL_DCLK_OUT);
>  	else
> -		clrbits_le32(&ccm->dram_clk_cfg, CCM_DRAM_CTRL_DCLK_OUT);
> +		clrbits_le32(&ccm->dram_clk_gate, CCM_DRAM_CTRL_DCLK_OUT);
>  #endif
>  }
>  
> diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> index 9dca800..6c0430c 100644
> --- a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> +++ b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> @@ -62,7 +62,7 @@ struct sunxi_ccm_reg {
>  	u32 gps_clk_cfg;	/* 0xd0 */
>  	u32 spi3_clk_cfg;	/* 0xd4 */
>  	u8 res5[0x28];
> -	u32 dram_clk_cfg;	/* 0x100 */
> +	u32 dram_clk_gate;	/* 0x100 */
>  	u32 be0_clk_cfg;	/* 0x104 */
>  	u32 be1_clk_cfg;	/* 0x108 */
>  	u32 fe0_clk_cfg;	/* 0x10c */

[U-Boot] [PATCH 2/6] sunxi: Add video pll clock functions

[Ian Campbell]

On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> This is a preparation patch for adding support for HDMI out.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ian Campbell <ijc@hellion.org.uk>

> ---
>  arch/arm/cpu/armv7/sunxi/clock_sun4i.c        | 27 ++++++++++++
>  arch/arm/cpu/armv7/sunxi/clock_sun6i.c        | 29 +++++++++++++
>  arch/arm/include/asm/arch-sunxi/clock.h       |  2 +
>  arch/arm/include/asm/arch-sunxi/clock_sun4i.h | 45 ++++++++++++++++++++
>  arch/arm/include/asm/arch-sunxi/clock_sun6i.h | 59 ++++++++++++++++++++++++++-
>  5 files changed, 161 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/clock_sun4i.c b/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
> index a0e49d1..49f4032 100644
> --- a/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
> +++ b/arch/arm/cpu/armv7/sunxi/clock_sun4i.c
> @@ -180,6 +180,21 @@ void clock_set_pll1(unsigned int hz)
>  }
>  #endif
>  
> +void clock_set_pll3(unsigned int clk)
> +{
> +	struct sunxi_ccm_reg * const ccm =
> +		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> +
> +	if (clk == 0) {
> +		clrbits_le32(&ccm->pll3_cfg, CCM_PLL3_CTRL_EN);
> +		return;
> +	}
> +
> +	/* PLL3 rate = 3000000 * m */
> +	writel(CCM_PLL3_CTRL_EN | CCM_PLL3_CTRL_INTEGER_MODE |
> +	       CCM_PLL3_CTRL_M(clk / 3000000), &ccm->pll3_cfg);
> +}
> +
>  unsigned int clock_get_pll5p(void)
>  {
>  	struct sunxi_ccm_reg *const ccm =
> @@ -200,3 +215,15 @@ unsigned int clock_get_pll6(void)
>  	int k = ((rval & CCM_PLL6_CTRL_K_MASK) >> CCM_PLL6_CTRL_K_SHIFT) + 1;
>  	return 24000000 * n * k / 2;
>  }
> +
> +void clock_set_de_mod_clock(u32 *clk_cfg, unsigned int hz)
> +{
> +	int pll = clock_get_pll5p();
> +	int div = 1;
> +
> +	while ((pll / div) > hz)
> +		div++;
> +
> +	writel(CCM_DE_CTRL_GATE | CCM_DE_CTRL_RST | CCM_DE_CTRL_PLL5P |
> +	       CCM_DE_CTRL_M(div), clk_cfg);
> +}
> diff --git a/arch/arm/cpu/armv7/sunxi/clock_sun6i.c b/arch/arm/cpu/armv7/sunxi/clock_sun6i.c
> index 16ab6f3..8e949c6 100644
> --- a/arch/arm/cpu/armv7/sunxi/clock_sun6i.c
> +++ b/arch/arm/cpu/armv7/sunxi/clock_sun6i.c
> @@ -127,6 +127,23 @@ void clock_set_pll1(unsigned int clk)
>  }
>  #endif
>  
> +void clock_set_pll3(unsigned int clk)
> +{
> +	struct sunxi_ccm_reg * const ccm =
> +		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> +	const int m = 8; /* 3 MHz steps just like sun4i, sun5i and sun7i */
> +
> +	if (clk == 0) {
> +		clrbits_le32(&ccm->pll3_cfg, CCM_PLL3_CTRL_EN);
> +		return;
> +	}
> +
> +	/* PLL3 rate = 24000000 * n / m */
> +	writel(CCM_PLL3_CTRL_EN | CCM_PLL3_CTRL_INTEGER_MODE |
> +	       CCM_PLL3_CTRL_N(clk / (24000000 / m)) | CCM_PLL3_CTRL_M(m),
> +	       &ccm->pll3_cfg);
> +}
> +
>  void clock_set_pll5(unsigned int clk)
>  {
>  	struct sunxi_ccm_reg * const ccm =
> @@ -151,3 +168,15 @@ unsigned int clock_get_pll6(void)
>  	int k = ((rval & CCM_PLL6_CTRL_K_MASK) >> CCM_PLL6_CTRL_K_SHIFT) + 1;
>  	return 24000000 * n * k / 2;
>  }
> +
> +void clock_set_de_mod_clock(u32 *clk_cfg, unsigned int hz)
> +{
> +	int pll = clock_get_pll6() * 2;
> +	int div = 1;
> +
> +	while ((pll / div) > hz)
> +		div++;
> +
> +	writel(CCM_DE_CTRL_GATE | CCM_DE_CTRL_PLL6_2X | CCM_DE_CTRL_M(div),
> +	       clk_cfg);
> +}
> diff --git a/arch/arm/include/asm/arch-sunxi/clock.h b/arch/arm/include/asm/arch-sunxi/clock.h
> index b40c16b..64acff3 100644
> --- a/arch/arm/include/asm/arch-sunxi/clock.h
> +++ b/arch/arm/include/asm/arch-sunxi/clock.h
> @@ -25,9 +25,11 @@
>  int clock_init(void);
>  int clock_twi_onoff(int port, int state);
>  void clock_set_pll1(unsigned int hz);
> +void clock_set_pll3(unsigned int hz);
>  void clock_set_pll5(unsigned int hz);
>  unsigned int clock_get_pll5p(void);
>  unsigned int clock_get_pll6(void);
> +void clock_set_de_mod_clock(u32 *clk_cfg, unsigned int hz);
>  void clock_init_safe(void);
>  void clock_init_uart(void);
>  #endif
> diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> index 6c0430c..eb88969 100644
> --- a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> +++ b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> @@ -186,12 +186,20 @@ struct sunxi_ccm_reg {
>  
>  /* ahb clock gate bit offset (second register) */
>  #define AHB_GATE_OFFSET_GMAC		17
> +#define AHB_GATE_OFFSET_DE_BE0		12
> +#define AHB_GATE_OFFSET_HDMI		11
> +#define AHB_GATE_OFFSET_LCD1		5
> +#define AHB_GATE_OFFSET_LCD0		4
>  
>  #define CCM_AHB_GATE_GPS (0x1 << 26)
>  #define CCM_AHB_GATE_SDRAM (0x1 << 14)
>  #define CCM_AHB_GATE_DLL (0x1 << 15)
>  #define CCM_AHB_GATE_ACE (0x1 << 16)
>  
> +#define CCM_PLL3_CTRL_M(n)		(((n) & 0x7f) << 0)
> +#define CCM_PLL3_CTRL_INTEGER_MODE	(0x1 << 15)
> +#define CCM_PLL3_CTRL_EN		(0x1 << 31)
> +
>  #define CCM_PLL5_CTRL_M(n) (((n) & 0x3) << 0)
>  #define CCM_PLL5_CTRL_M_MASK CCM_PLL5_CTRL_M(0x3)
>  #define CCM_PLL5_CTRL_M_X(n) ((n) - 1)
> @@ -253,6 +261,34 @@ struct sunxi_ccm_reg {
>  
>  #define CCM_MMC_CTRL_ENABLE (0x1 << 31)
>  
> +#define CCM_DRAM_GATE_OFFSET_DE_BE0	26
> +
> +#define CCM_LCD_CH0_CTRL_PLL3		(0 << 24)
> +#define CCM_LCD_CH0_CTRL_PLL7		(1 << 24)
> +#define CCM_LCD_CH0_CTRL_PLL3_2X	(2 << 24)
> +#define CCM_LCD_CH0_CTRL_PLL7_2X	(3 << 24)
> +#define CCM_LCD_CH0_CTRL_RST		(0x1 << 30)
> +#define CCM_LCD_CH0_CTRL_GATE		(0x1 << 31)
> +
> +#define CCM_LCD_CH1_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
> +/* We leave bit 11 set to 0, so sclk1 == sclk2 */
> +#define CCM_LCD_CH1_CTRL_PLL3		(0 << 24)
> +#define CCM_LCD_CH1_CTRL_PLL7		(1 << 24)
> +#define CCM_LCD_CH1_CTRL_PLL3_2X	(2 << 24)
> +#define CCM_LCD_CH1_CTRL_PLL7_2X	(3 << 24)
> +/* Enable / disable both ch1 sclk1 and sclk2 at the same time */
> +#define CCM_LCD_CH1_CTRL_GATE		(0x1 << 31 | 0x1 << 15)
> +
> +#define CCM_HDMI_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
> +#define CCM_HDMI_CTRL_PLL_MASK		(3 << 24)
> +#define CCM_HDMI_CTRL_PLL3		(0 << 24)
> +#define CCM_HDMI_CTRL_PLL7		(1 << 24)
> +#define CCM_HDMI_CTRL_PLL3_2X		(2 << 24)
> +#define CCM_HDMI_CTRL_PLL7_2X		(3 << 24)
> +/* No separate ddc gate on sun4i, sun5i and sun7i */
> +#define CCM_HDMI_CTRL_DDC_GATE		0
> +#define CCM_HDMI_CTRL_GATE		(0x1 << 31)
> +
>  #define CCM_GMAC_CTRL_TX_CLK_SRC_MII 0x0
>  #define CCM_GMAC_CTRL_TX_CLK_SRC_EXT_RGMII 0x1
>  #define CCM_GMAC_CTRL_TX_CLK_SRC_INT_RGMII 0x2
> @@ -266,4 +302,13 @@ struct sunxi_ccm_reg {
>  #define CCM_USB_CTRL_PHY1_CLK 0
>  #define CCM_USB_CTRL_PHY2_CLK 0
>  
> +/* CCM bits common to all Display Engine (and IEP) clock ctrl regs */
> +#define CCM_DE_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
> +#define CCM_DE_CTRL_PLL_MASK		(3 << 24)
> +#define CCM_DE_CTRL_PLL3		(0 << 24)
> +#define CCM_DE_CTRL_PLL7		(1 << 24)
> +#define CCM_DE_CTRL_PLL5P		(2 << 24)
> +#define CCM_DE_CTRL_RST			(1 << 30)
> +#define CCM_DE_CTRL_GATE		(1 << 31)
> +
>  #endif /* _SUNXI_CLOCK_SUN4I_H */
> diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> index e16a764..50a4b69 100644
> --- a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> +++ b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> @@ -176,13 +176,18 @@ struct sunxi_ccm_reg {
>  #define CCM_PLL1_CTRL_MAGIC		(0x1 << 16)
>  #define CCM_PLL1_CTRL_EN		(0x1 << 31)
>  
> +#define CCM_PLL3_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
> +#define CCM_PLL3_CTRL_N(n)		((((n) - 1) & 0x7f) << 8)
> +#define CCM_PLL3_CTRL_INTEGER_MODE	(0x1 << 24)
> +#define CCM_PLL3_CTRL_EN		(0x1 << 31)
> +
>  #define CCM_PLL5_CTRL_M(n)		((((n) - 1) & 0x3) << 0)
>  #define CCM_PLL5_CTRL_K(n)		((((n) - 1) & 0x3) << 4)
>  #define CCM_PLL5_CTRL_N(n)		((((n) - 1) & 0x1f) << 8)
>  #define CCM_PLL5_CTRL_UPD		(0x1 << 20)
>  #define CCM_PLL5_CTRL_EN		(0x1 << 31)
>  
> -#define PLL6_CFG_DEFAULT		0x90041811
> +#define PLL6_CFG_DEFAULT		0x90041811 /* 600 MHz */
>  
>  #define CCM_PLL6_CTRL_N_SHIFT		8
>  #define CCM_PLL6_CTRL_N_MASK		(0x1f << CCM_PLL6_CTRL_N_SHIFT)
> @@ -193,6 +198,7 @@ struct sunxi_ccm_reg {
>  
>  #define AXI_GATE_OFFSET_DRAM		0
>  
> +/* ahb_gate0 offsets */
>  #define AHB_GATE_OFFSET_USB_OHCI1	30
>  #define AHB_GATE_OFFSET_USB_OHCI0	29
>  #define AHB_GATE_OFFSET_USB_EHCI1	27
> @@ -204,6 +210,13 @@ struct sunxi_ccm_reg {
>  #define AHB_GATE_OFFSET_MMC0		8
>  #define AHB_GATE_OFFSET_MMC(n)		(AHB_GATE_OFFSET_MMC0 + (n))
>  
> +/* ahb_gate1 offsets */
> +#define AHB_GATE_OFFSET_DRC0		25
> +#define AHB_GATE_OFFSET_DE_BE0		12
> +#define AHB_GATE_OFFSET_HDMI		11
> +#define AHB_GATE_OFFSET_LCD1		5
> +#define AHB_GATE_OFFSET_LCD0		4
> +
>  #define CCM_MMC_CTRL_OSCM24 (0x0 << 24)
>  #define CCM_MMC_CTRL_PLL6   (0x1 << 24)
>  
> @@ -223,8 +236,34 @@ struct sunxi_ccm_reg {
>  #define CCM_DRAMCLK_CFG_UPD		(0x1 << 16)
>  #define CCM_DRAMCLK_CFG_RST		(0x1 << 31)
>  
> +#define CCM_DRAM_GATE_OFFSET_DE_BE0	26
> +
> +#define CCM_LCD_CH0_CTRL_PLL3		(0 << 24)
> +#define CCM_LCD_CH0_CTRL_PLL7		(1 << 24)
> +#define CCM_LCD_CH0_CTRL_PLL3_2X	(2 << 24)
> +#define CCM_LCD_CH0_CTRL_PLL7_2X	(3 << 24)
> +#define CCM_LCD_CH0_CTRL_MIPI_PLL	(4 << 24)
> +#define CCM_LCD_CH0_CTRL_GATE		(0x1 << 31)
> +
> +#define CCM_LCD_CH1_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
> +#define CCM_LCD_CH1_CTRL_PLL3		(0 << 24)
> +#define CCM_LCD_CH1_CTRL_PLL7		(1 << 24)
> +#define CCM_LCD_CH1_CTRL_PLL3_2X	(2 << 24)
> +#define CCM_LCD_CH1_CTRL_PLL7_2X	(3 << 24)
> +#define CCM_LCD_CH1_CTRL_GATE		(0x1 << 31)
> +
> +#define CCM_HDMI_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
> +#define CCM_HDMI_CTRL_PLL_MASK		(3 << 24)
> +#define CCM_HDMI_CTRL_PLL3		(0 << 24)
> +#define CCM_HDMI_CTRL_PLL7		(1 << 24)
> +#define CCM_HDMI_CTRL_PLL3_2X		(2 << 24)
> +#define CCM_HDMI_CTRL_PLL7_2X		(3 << 24)
> +#define CCM_HDMI_CTRL_DDC_GATE		(0x1 << 30)
> +#define CCM_HDMI_CTRL_GATE		(0x1 << 31)
> +
>  #define MBUS_CLK_DEFAULT		0x81000001 /* PLL6 / 2 */
>  
> +/* ahb_reset0 offsets */
>  #define AHB_RESET_OFFSET_MCTL		14
>  #define AHB_RESET_OFFSET_MMC3		11
>  #define AHB_RESET_OFFSET_MMC2		10
> @@ -232,10 +271,28 @@ struct sunxi_ccm_reg {
>  #define AHB_RESET_OFFSET_MMC0		8
>  #define AHB_RESET_OFFSET_MMC(n)		(AHB_RESET_OFFSET_MMC0 + (n))
>  
> +/* ahb_reset0 offsets */
> +#define AHB_RESET_OFFSET_DRC0		25
> +#define AHB_RESET_OFFSET_DE_BE0		12
> +#define AHB_RESET_OFFSET_HDMI		11
> +#define AHB_RESET_OFFSET_LCD1		5
> +#define AHB_RESET_OFFSET_LCD0		4
> +
>  /* apb2 reset */
>  #define APB2_RESET_UART_SHIFT		(16)
>  #define APB2_RESET_UART_MASK		(0xff << APB2_RESET_UART_SHIFT)
>  #define APB2_RESET_TWI_SHIFT		(0)
>  #define APB2_RESET_TWI_MASK		(0xf << APB2_RESET_TWI_SHIFT)
>  
> +/* CCM bits common to all Display Engine (and IEP) clock ctrl regs */
> +#define CCM_DE_CTRL_M(n)		((((n) - 1) & 0xf) << 0)
> +#define CCM_DE_CTRL_PLL_MASK		(0xf << 24)
> +#define CCM_DE_CTRL_PLL3		(0 << 24)
> +#define CCM_DE_CTRL_PLL7		(1 << 24)
> +#define CCM_DE_CTRL_PLL6_2X		(2 << 24)
> +#define CCM_DE_CTRL_PLL8		(3 << 24)
> +#define CCM_DE_CTRL_PLL9		(4 << 24)
> +#define CCM_DE_CTRL_PLL10		(5 << 24)
> +#define CCM_DE_CTRL_GATE		(1 << 31)
> +
>  #endif /* _SUNXI_CLOCK_SUN6I_H */

[U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi

[Ian Campbell]

On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> +/* this is needed so the above will actually do something */
> +#define CONFIG_SYS_CONSOLE_IS_IN_ENV
> [...] 
> +#ifdef CONFIG_VIDEO
> +#define CONSOLE_ENV_SETTINGS \
> +	"stdin=serial\0" \
> +	"stdout=serial,vga\0" \
> +	"stderr=serial,vga\0"
> +#else
> +#define CONSOLE_ENV_SETTINGS

Now that CONFIG_SYS_CONSOLE_IS_IN_ENV is set do we need to either say
something here and/or provide the overwrite_console() hook?

Perhaps something along the lines of:
        #ifdef CONFIG_VIDEO
        #define CONSOLE_ENV_VGA_SETTINGS ",vga"
        #endif
        
        #define CONSOLE_ENV_SETTINGS \
        	"stdin=serial\0" \
        	"stdout=serial" CONSOLE_ENV_VGA_SETTINGS "\0" \
        	"stderr=serial" CONSOLE_ENV_VGA_SETTINGS "\0"
         
Has this been tested on a serial-only board?

Ian.

[U-Boot] [PATCH 4/6] sunxi: video: Add sun6i support

[Ian Campbell]

On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> @@ -177,7 +188,11 @@ sunxi_lcdc_init(void)
>  		(struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
>  
>  	/* Reset off */
> +#ifdef CONFIG_MACH_SUN6I
> +	setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_LCD0);
> +#else
>  	setbits_le32(&ccm->lcd0_ch0_clk_cfg, CCM_LCD_CH0_CTRL_RST);
> +#endif

If you are sure it is correct to no longer frob the lcd clk for sun6i
here (which seems a little odd, might be worth mentioning in the commit
log if it is right) then:

Acked-by: Ian Campbell <ijc@hellion.org.uk>

Ian.

[U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi

[Ian Campbell]

On Sun, 2014-11-16 at 11:35 +0000, Ian Campbell wrote:
> On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> > +/* this is needed so the above will actually do something */
> > +#define CONFIG_SYS_CONSOLE_IS_IN_ENV
> > [...] 
> > +#ifdef CONFIG_VIDEO
> > +#define CONSOLE_ENV_SETTINGS \
> > +	"stdin=serial\0" \
> > +	"stdout=serial,vga\0" \
> > +	"stderr=serial,vga\0"
> > +#else
> > +#define CONSOLE_ENV_SETTINGS
> 
> Now that CONFIG_SYS_CONSOLE_IS_IN_ENV is set do we need to either say
> something here and/or provide the overwrite_console() hook?
> 
> Perhaps something along the lines of:
>         #ifdef CONFIG_VIDEO
>         #define CONSOLE_ENV_VGA_SETTINGS ",vga"
>         #endif
>         
>         #define CONSOLE_ENV_SETTINGS \
>         	"stdin=serial\0" \
>         	"stdout=serial" CONSOLE_ENV_VGA_SETTINGS "\0" \
>         	"stderr=serial" CONSOLE_ENV_VGA_SETTINGS "\0"
>          
> Has this been tested on a serial-only board?

Forgot to say: all the rest looks fine to me and/or I'm satisfied if
Anatolij is.

Ian

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Ian Campbell]

On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> From: Luc Verhaegen <libv@skynet.be>
> 
> Add simplefb support, note this depends on the kernel having support for
> the clocks property which has recently been added to the simplefb devicetree
> binding.

Link please, Linus's tree[0] doesn't seem to have it yet, I suppose it
is in some maintainer tree at the moment? It's not even in linux-next
yet though[1]. Maybe simple-framebuffer.txt isn't the right place to
look?

[0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
[1] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt

> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
> +void
> +sunxi_simplefb_setup(void *blob)
> +{
> +	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
> +	const char *node = "/chosen/framebuffer0-hdmi";
> +	const char *format = "x8r8g8b8";

The bindings doc currently says:
        
        - format: The format of the framebuffer surface. Valid values are:
          - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
          - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
        
Perhaps x8r8g8b8 is defined in the updated version?

> +	const char *okay = "okay";
> +	char name[32];
> +	fdt32_t cells[2];
> +	int offset, stride, ret;
> +
> +	if (!sunxi_display.enabled)
> +		return;
> +
> +	offset = fdt_path_offset(blob, node);

I think you should use fdt_node_offset_by_compatible here instead of
hardcoding a path. common/lcd.c does it this way too, it also doesn't
appear to use a node under /chosen. Perhaps this is changed in the more
uptodate binding which I've not seen yet.

> +	cells[0] = cpu_to_fdt32(gd->fb_base);
> +	cells[1] = cpu_to_fdt32(CONFIG_SUNXI_FB_SIZE);
> +	ret = fdt_setprop(blob, offset, "reg", cells, sizeof(cells[0]) * 2);

What arranges that #address-cells and #size-cells are both 1 at this
point? Does u-boot not have a helper to setup a cells array including
looking those up?

Also the bindings doc seems to imply that size should be the actual
configured size of the video ram region ("(1600 * 1200 * 2)") rather
than the size of the reserved memory region. Maybe it's not important.

> +	ret = fdt_setprop(blob, offset, "format", format, strlen(format) + 1);
> +	if (ret < 0)
> +		goto error;
> +
> +	ret = fdt_setprop(blob, offset, "status", okay, strlen(okay) + 1);

You can use fdt_setprop_string for these two, I think.

Ian.

[U-Boot] [PATCH 6/6] sunxi: Add usb keyboard Kconfig option

[Ian Campbell]

On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> For use together with the hdmi console.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  board/sunxi/Kconfig            |  7 +++++++
>  configs/Ippo_q8h_v5_defconfig  |  1 +
>  include/configs/sunxi-common.h | 25 +++++++++++++++++++++----
>  3 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
> index 422033a..246cd9a 100644
> --- a/board/sunxi/Kconfig
> +++ b/board/sunxi/Kconfig
> @@ -223,4 +223,11 @@ config VIDEO
>  	Say Y here to add support for using a cfb console on the HDMI output
>  	found on most sunxi devices.
>  
> +config USB_KEYBOARD

This seems like it ought to be under drivers/ somewhere, either
drivers/usb/Kconfig or drivers/input/Kconfig perhaps.

> +	boolean "Enable USB keyboard support"
> +	default y
> +	---help---
> +	Say Y here to add support for using a USB keyboard (typically used
> +	in combination with a graphical console on HDMI).
> +
>  endif
> diff --git a/configs/Ippo_q8h_v5_defconfig b/configs/Ippo_q8h_v5_defconfig
> index 53df213..50c2f93 100644
> --- a/configs/Ippo_q8h_v5_defconfig
> +++ b/configs/Ippo_q8h_v5_defconfig
> @@ -5,3 +5,4 @@ CONFIG_MACH_SUN8I=y
>  CONFIG_TARGET_IPPO_Q8H_V5=y
>  CONFIG_DEFAULT_DEVICE_TREE="sun8i-a23-ippo-q8h-v5.dtb"
>  CONFIG_VIDEO=n
> +CONFIG_USB_KEYBOARD=n

Is this the only platform with video+usb which you have? What about e.g.
cubie*, bananapi etc?

> @@ -298,17 +304,28 @@
>  
>  #include <config_distro_bootcmd.h>
>  
> +#ifdef CONFIG_USB_KEYBOARD
> +#define CONSOLE_IN_SETTINGS \
> +	"preboot=usb start\0" \
> +	"stdin=serial,usbkbd\0"
> +#else
> +#define CONSOLE_IN_SETTINGS \
> +	"stdin=serial\0"
> +#endif
> +
>  #ifdef CONFIG_VIDEO
> -#define CONSOLE_ENV_SETTINGS \
> -	"stdin=serial\0" \
> +#define CONSOLE_OUT_SETTINGS \
>  	"stdout=serial,vga\0" \
>  	"stderr=serial,vga\0"
>  #else
> -#define CONSOLE_ENV_SETTINGS
> +#define CONSOLE_OUT_SETTINGS \
> +	"stdout=serial\0" \
> +	"stderr=serial\0"

Ah, here are the settings I asked about earlier. Are these in the wrong
patch or did something change in this patch which makes them needed only
now?

I'm not sure but I wonder if the cpp string pasting thing I suggested
earlier would reduce the amount of #else and duplication around here?

>  #endif
>  
>  #define CONFIG_EXTRA_ENV_SETTINGS \
> -	CONSOLE_ENV_SETTINGS \
> +	CONSOLE_IN_SETTINGS \
> +	CONSOLE_OUT_SETTINGS \

I suggest to #define CONSOLE_ENV_SETTINGS as the other two, and add STD
to their names.

>  	MEM_LAYOUT_ENV_SETTINGS \
>  	"fdtfile=" CONFIG_FDTFILE "\0" \
>  	"console=ttyS0,115200\0" \

[U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi

[Hans de Goede]

Hi,

Thanks for the reviews!


On 11/16/2014 12:35 PM, Ian Campbell wrote:
> On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
>> +/* this is needed so the above will actually do something */
>> +#define CONFIG_SYS_CONSOLE_IS_IN_ENV
>> [...] 
>> +#ifdef CONFIG_VIDEO
>> +#define CONSOLE_ENV_SETTINGS \
>> +	"stdin=serial\0" \
>> +	"stdout=serial,vga\0" \
>> +	"stderr=serial,vga\0"
>> +#else
>> +#define CONSOLE_ENV_SETTINGS
> 
> Now that CONFIG_SYS_CONSOLE_IS_IN_ENV is set do we need to either say
> something here and/or provide the overwrite_console() hook?

No, if we have a need for a overwrite_console() hook, we need to also
define CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE, see common/console.c:

#ifdef CONFIG_SYS_CONSOLE_IS_IN_ENV
/*
 * if overwrite_console returns 1, the stdin, stderr and stdout
 * are switched to the serial port, else the settings in the
 * environment are used
 */
#ifdef CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE
extern int overwrite_console(void);
#define OVERWRITE_CONSOLE overwrite_console()
#else
#define OVERWRITE_CONSOLE 0
#endif /* CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE */

#endif /* CONFIG_SYS_CONSOLE_IS_IN_ENV */

> Perhaps something along the lines of:
>         #ifdef CONFIG_VIDEO
>         #define CONSOLE_ENV_VGA_SETTINGS ",vga"
>         #endif
>         
>         #define CONSOLE_ENV_SETTINGS \
>         	"stdin=serial\0" \
>         	"stdout=serial" CONSOLE_ENV_VGA_SETTINGS "\0" \
>         	"stderr=serial" CONSOLE_ENV_VGA_SETTINGS "\0"

There is no need to set these when CONFIG_VIDEO is not defined
since CONFIG_SYS_CONSOLE_IS_IN_ENV only gets set when
CONFIG_VIDEO is set, at least in this point in the patch set.

Once we add usb-keyboard support I want to support usb-keyb. support,
without having it depend on CONFIG_VIDEO (even though it makes the
most sense with CONFIG_VIDEO).

But even then I prefer this style:

#ifdef CONFIG_VIDEO
#define CONSOLE_OUT_SETTINGS \
        "stdout=serial,vga\0" \
        "stderr=serial,vga\0"
#else
#define CONSOLE_OUT_SETTINGS \
        "stdout=serial\0" \
        "stderr=serial\0"
#endif

Over the pre-processor magic you're suggesting, as it is more
readable IMHO, also it matches what we're do for
#ifdef CONFIG_MMC, #ifdef CONFIG_AHCI, etc.

> Has this been tested on a serial-only board?

Erm, good question.

So the A13 olinuxino boards do not have hdmi, which means I should
add CONFIG_VIDEO=n to their defconfigs, fixed in my local tree.

After that I've just ran some tests on an A13 board, which work fine
(as expected, but you're right this ought to be tested).

Regards,

Hans

[U-Boot] [PATCH 6/6] sunxi: Add usb keyboard Kconfig option

[Hans de Goede]

Hi,

On 11/16/2014 12:55 PM, Ian Campbell wrote:
> On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
>> For use together with the hdmi console.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  board/sunxi/Kconfig            |  7 +++++++
>>  configs/Ippo_q8h_v5_defconfig  |  1 +
>>  include/configs/sunxi-common.h | 25 +++++++++++++++++++++----
>>  3 files changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
>> index 422033a..246cd9a 100644
>> --- a/board/sunxi/Kconfig
>> +++ b/board/sunxi/Kconfig
>> @@ -223,4 +223,11 @@ config VIDEO
>>  	Say Y here to add support for using a cfb console on the HDMI output
>>  	found on most sunxi devices.
>>  
>> +config USB_KEYBOARD
> 
> This seems like it ought to be under drivers/ somewhere, either
> drivers/usb/Kconfig or drivers/input/Kconfig perhaps.

You're right the problem with that is, that what we really need is
to Kconfig-ify the entire USB stuff (eg also CONFIG_USB_EHCI), but that
is a bit more then I was planning on working on atm, esp. since that
touches many many boards. So this seems best for now.

> 
>> +	boolean "Enable USB keyboard support"
>> +	default y
>> +	---help---
>> +	Say Y here to add support for using a USB keyboard (typically used
>> +	in combination with a graphical console on HDMI).
>> +
>>  endif
>> diff --git a/configs/Ippo_q8h_v5_defconfig b/configs/Ippo_q8h_v5_defconfig
>> index 53df213..50c2f93 100644
>> --- a/configs/Ippo_q8h_v5_defconfig
>> +++ b/configs/Ippo_q8h_v5_defconfig
>> @@ -5,3 +5,4 @@ CONFIG_MACH_SUN8I=y
>>  CONFIG_TARGET_IPPO_Q8H_V5=y
>>  CONFIG_DEFAULT_DEVICE_TREE="sun8i-a23-ippo-q8h-v5.dtb"
>>  CONFIG_VIDEO=n
>> +CONFIG_USB_KEYBOARD=n
> 
> Is this the only platform with video+usb which you have? What about e.g.
> cubie*, bananapi etc?

You mean without video + usb, since the default is y, and it is being
forced to n here. No this is not the only board we support without HDMI,
I has forgotten about the A13 boards (fixed locally now), this is the
only board without a host usb connector (it is a tablet), it does support
otg, but we do not support that yet.

I've also made a local change to disable CONFIG_USB_KEYBOARD by default
in the A13 boards since although it does work there it makes little
sense to have it when their is no video out support.

>> @@ -298,17 +304,28 @@
>>  
>>  #include <config_distro_bootcmd.h>
>>  
>> +#ifdef CONFIG_USB_KEYBOARD
>> +#define CONSOLE_IN_SETTINGS \
>> +	"preboot=usb start\0" \
>> +	"stdin=serial,usbkbd\0"
>> +#else
>> +#define CONSOLE_IN_SETTINGS \
>> +	"stdin=serial\0"
>> +#endif
>> +
>>  #ifdef CONFIG_VIDEO
>> -#define CONSOLE_ENV_SETTINGS \
>> -	"stdin=serial\0" \
>> +#define CONSOLE_OUT_SETTINGS \
>>  	"stdout=serial,vga\0" \
>>  	"stderr=serial,vga\0"
>>  #else
>> -#define CONSOLE_ENV_SETTINGS
>> +#define CONSOLE_OUT_SETTINGS \
>> +	"stdout=serial\0" \
>> +	"stderr=serial\0"
> 
> Ah, here are the settings I asked about earlier. Are these in the wrong
> patch or did something change in this patch which makes them needed only
> now?

These are only necessary when CONFIG_SYS_CONSOLE_IS_IN_ENV is set, which
sofar was only happening when CONFIG_VIDEO is set. In this patch I'm setting
them always to also support just CONFIG_USB_KEYBOARD without CONFIG_VIDEO,
which is a bit weird, but I do not want to disallow it.

This does mean that we should also unconditionally enable
CONFIG_SYS_CONSOLE_IS_IN_ENV with this patch (moving it out of the
#ifdef CONFIG_VIDEO) block, which I've not done, I'll fix this locally.

> I'm not sure but I wonder if the cpp string pasting thing I suggested
> earlier would reduce the amount of #else and duplication around here?

See my previous mail (I would like to keep this as is).


> 
>>  #endif
>>  
>>  #define CONFIG_EXTRA_ENV_SETTINGS \
>> -	CONSOLE_ENV_SETTINGS \
>> +	CONSOLE_IN_SETTINGS \
>> +	CONSOLE_OUT_SETTINGS \
> 
> I suggest to #define CONSOLE_ENV_SETTINGS as the other two, and add STD
> to their names.

Fixed in my local tree.

Regards,

Hans

[U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi

[Ian Campbell]

On Sun, 2014-11-16 at 14:15 +0100, Hans de Goede wrote:
> Hi,
> 
> Thanks for the reviews!
> 
> 
> On 11/16/2014 12:35 PM, Ian Campbell wrote:
> > On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> >> +/* this is needed so the above will actually do something */
> >> +#define CONFIG_SYS_CONSOLE_IS_IN_ENV
> >> [...] 
> >> +#ifdef CONFIG_VIDEO
> >> +#define CONSOLE_ENV_SETTINGS \
> >> +	"stdin=serial\0" \
> >> +	"stdout=serial,vga\0" \
> >> +	"stderr=serial,vga\0"
> >> +#else
> >> +#define CONSOLE_ENV_SETTINGS
> > 
> > Now that CONFIG_SYS_CONSOLE_IS_IN_ENV is set do we need to either say
> > something here and/or provide the overwrite_console() hook?
> 
> No, if we have a need for a overwrite_console() hook, we need to also
> define CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE, see common/console.c:

Ah, README didn't mention this under CONSOLE_IS_IN_ENV, only under
OVERWRITE_ROUTINE which I didn't know to look for.

> But even then I prefer this style:
> 
> #ifdef CONFIG_VIDEO
> #define CONSOLE_OUT_SETTINGS \
>         "stdout=serial,vga\0" \
>         "stderr=serial,vga\0"
> #else
> #define CONSOLE_OUT_SETTINGS \
>         "stdout=serial\0" \
>         "stderr=serial\0"
> #endif
> 
> Over the pre-processor magic you're suggesting, as it is more
> readable IMHO, also it matches what we're do for
> #ifdef CONFIG_MMC, #ifdef CONFIG_AHCI, etc.

OK.

> 
> > Has this been tested on a serial-only board?
> 
> Erm, good question.
> 
> So the A13 olinuxino boards do not have hdmi, which means I should
> add CONFIG_VIDEO=n to their defconfigs, fixed in my local tree.
> 
> After that I've just ran some tests on an A13 board, which work fine
> (as expected, but you're right this ought to be tested).

Great, thanks,

Acked-by: Ian Campbell <ijc@hellion.org.uk>

[U-Boot] [PATCH 6/6] sunxi: Add usb keyboard Kconfig option

[Ian Campbell]

On Sun, 2014-11-16 at 14:28 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/16/2014 12:55 PM, Ian Campbell wrote:
> > On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> >> For use together with the hdmi console.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  board/sunxi/Kconfig            |  7 +++++++
> >>  configs/Ippo_q8h_v5_defconfig  |  1 +
> >>  include/configs/sunxi-common.h | 25 +++++++++++++++++++++----
> >>  3 files changed, 29 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
> >> index 422033a..246cd9a 100644
> >> --- a/board/sunxi/Kconfig
> >> +++ b/board/sunxi/Kconfig
> >> @@ -223,4 +223,11 @@ config VIDEO
> >>  	Say Y here to add support for using a cfb console on the HDMI output
> >>  	found on most sunxi devices.
> >>  
> >> +config USB_KEYBOARD
> > 
> > This seems like it ought to be under drivers/ somewhere, either
> > drivers/usb/Kconfig or drivers/input/Kconfig perhaps.
> 
> You're right the problem with that is, that what we really need is
> to Kconfig-ify the entire USB stuff (eg also CONFIG_USB_EHCI), but that
> is a bit more then I was planning on working on atm, esp. since that
> touches many many boards. So this seems best for now.

OK, is the USB custodian OK with this plan?

> >> +	boolean "Enable USB keyboard support"
> >> +	default y
> >> +	---help---
> >> +	Say Y here to add support for using a USB keyboard (typically used
> >> +	in combination with a graphical console on HDMI).
> >> +
> >>  endif
> >> diff --git a/configs/Ippo_q8h_v5_defconfig b/configs/Ippo_q8h_v5_defconfig
> >> index 53df213..50c2f93 100644
> >> --- a/configs/Ippo_q8h_v5_defconfig
> >> +++ b/configs/Ippo_q8h_v5_defconfig
> >> @@ -5,3 +5,4 @@ CONFIG_MACH_SUN8I=y
> >>  CONFIG_TARGET_IPPO_Q8H_V5=y
> >>  CONFIG_DEFAULT_DEVICE_TREE="sun8i-a23-ippo-q8h-v5.dtb"
> >>  CONFIG_VIDEO=n
> >> +CONFIG_USB_KEYBOARD=n
> > 
> > Is this the only platform with video+usb which you have? What about e.g.
> > cubie*, bananapi etc?
> 
> You mean without video + usb, since the default is y, and it is being
> forced to n here.

Sorry, I read this backwards somehow!

> No this is not the only board we support without HDMI,
> I has forgotten about the A13 boards (fixed locally now), this is the
> only board without a host usb connector (it is a tablet), it does support
> otg, but we do not support that yet.
> 
> I've also made a local change to disable CONFIG_USB_KEYBOARD by default
> in the A13 boards since although it does work there it makes little
> sense to have it when their is no video out support.

Sounds good.

> >> @@ -298,17 +304,28 @@
> >>  
> >>  #include <config_distro_bootcmd.h>
> >>  
> >> +#ifdef CONFIG_USB_KEYBOARD
> >> +#define CONSOLE_IN_SETTINGS \
> >> +	"preboot=usb start\0" \
> >> +	"stdin=serial,usbkbd\0"
> >> +#else
> >> +#define CONSOLE_IN_SETTINGS \
> >> +	"stdin=serial\0"
> >> +#endif
> >> +
> >>  #ifdef CONFIG_VIDEO
> >> -#define CONSOLE_ENV_SETTINGS \
> >> -	"stdin=serial\0" \
> >> +#define CONSOLE_OUT_SETTINGS \
> >>  	"stdout=serial,vga\0" \
> >>  	"stderr=serial,vga\0"
> >>  #else
> >> -#define CONSOLE_ENV_SETTINGS
> >> +#define CONSOLE_OUT_SETTINGS \
> >> +	"stdout=serial\0" \
> >> +	"stderr=serial\0"
> > 
> > Ah, here are the settings I asked about earlier. Are these in the wrong
> > patch or did something change in this patch which makes them needed only
> > now?
> 
> These are only necessary when CONFIG_SYS_CONSOLE_IS_IN_ENV is set, which
> sofar was only happening when CONFIG_VIDEO is set. In this patch I'm setting
> them always to also support just CONFIG_USB_KEYBOARD without CONFIG_VIDEO,
> which is a bit weird, but I do not want to disallow it.
> 
> This does mean that we should also unconditionally enable
> CONFIG_SYS_CONSOLE_IS_IN_ENV with this patch (moving it out of the
> #ifdef CONFIG_VIDEO) block, which I've not done, I'll fix this locally.

OK. Not sure what you were planning but it may makes sense to fold some
portion of that change into the earlier patch.

> > I'm not sure but I wonder if the cpp string pasting thing I suggested
> > earlier would reduce the amount of #else and duplication around here?
> 
> See my previous mail (I would like to keep this as is).

Sure.

The changes you proposed here all sound fine, I'll defer a formal ack
till I've seen them though.

Ian.

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Hans de Goede]

Hi,

On 11/16/2014 12:50 PM, Ian Campbell wrote:
> On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
>> From: Luc Verhaegen <libv@skynet.be>
>>
>> Add simplefb support, note this depends on the kernel having support for
>> the clocks property which has recently been added to the simplefb devicetree
>> binding.
> 
> Link please, Linus's tree[0] doesn't seem to have it yet, I suppose it
> is in some maintainer tree at the moment? It's not even in linux-next
> yet though[1]. Maybe simple-framebuffer.txt isn't the right place to
> look?

Right now it is sitting here, which is the fbdev maintainers official tree:
https://git.kernel.org/cgit/linux/kernel/git/tomba/linux.git/log/?h=for-next

> 
> [0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> [1] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> 
>> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
>> +void
>> +sunxi_simplefb_setup(void *blob)
>> +{
>> +	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
>> +	const char *node = "/chosen/framebuffer0-hdmi";
>> +	const char *format = "x8r8g8b8";
> 
> The bindings doc currently says:
>         
>         - format: The format of the framebuffer surface. Valid values are:
>           - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>           - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>         
> Perhaps x8r8g8b8 is defined in the updated version?

Erm, no, I don't think anyone has actually bothered to keep the list in the
binding up2date with what the kernel actually supports, x8r8g8b8 has been
supported in the kernel before sunxi + simplefb every became a topic.

> 
>> +	const char *okay = "okay";
>> +	char name[32];
>> +	fdt32_t cells[2];
>> +	int offset, stride, ret;
>> +
>> +	if (!sunxi_display.enabled)
>> +		return;
>> +
>> +	offset = fdt_path_offset(blob, node);
> 
> I think you should use fdt_node_offset_by_compatible here instead of
> hardcoding a path.

Hardcoding a path is deliberate. I don't know if you've read the
previous u-boot code for this, but it did a lot of dt parsing to
find clocks and add phandles to them, the way we eventually settled
on when discussing this is for the dts to contain pre-populated simplefb
nodes which u-boot just needs to fill with the mode info and enable, this
way as we add support for more clocks (like the module clocks for the
various display pipeline blocks), we don't need to update u-boot in lock-step,
we just add the clocks to the simplefb node in the dts file when they get
added to the dts file in the first place. See the updated bindings.

> common/lcd.c does it this way too, it also doesn't
> appear to use a node under /chosen. Perhaps this is changed in the more
> uptodate binding which I've not seen yet.

The use of /chosen is part of the updated bindings, which were discussed
in length on various lists, and then eventually I spend 2 days online with
Grant Likely in #devicetree to hash things out.

>> +	cells[0] = cpu_to_fdt32(gd->fb_base);
>> +	cells[1] = cpu_to_fdt32(CONFIG_SUNXI_FB_SIZE);
>> +	ret = fdt_setprop(blob, offset, "reg", cells, sizeof(cells[0]) * 2);
> 
> What arranges that #address-cells and #size-cells are both 1 at this
> point?

The pre-filled simplefb node.

> Does u-boot not have a helper to setup a cells array including
> looking those up?

Good question.

/me looks

Doesn't look like it, what we've available basically is a bare libfdt,
and it does not look like that can do this.

> Also the bindings doc seems to imply that size should be the actual
> configured size of the video ram region ("(1600 * 1200 * 2)") rather
> than the size of the reserved memory region. Maybe it's not important.

Heh, it is not important really / does not matter either way.

I actually tried fixing the example, in the bindings but people found it
more clear as it is.

> 
>> +	ret = fdt_setprop(blob, offset, "format", format, strlen(format) + 1);
>> +	if (ret < 0)
>> +		goto error;
>> +
>> +	ret = fdt_setprop(blob, offset, "status", okay, strlen(okay) + 1);
> 
> You can use fdt_setprop_string for these two, I think.

Yes, good one, will fix in my local tree.

Regards,

Hans

[U-Boot] [PATCH 6/6] sunxi: Add usb keyboard Kconfig option

[Hans de Goede]

Hi,

On 11/16/2014 02:37 PM, Ian Campbell wrote:
> On Sun, 2014-11-16 at 14:28 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/16/2014 12:55 PM, Ian Campbell wrote:
>>> On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
>>>> For use together with the hdmi console.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  board/sunxi/Kconfig            |  7 +++++++
>>>>  configs/Ippo_q8h_v5_defconfig  |  1 +
>>>>  include/configs/sunxi-common.h | 25 +++++++++++++++++++++----
>>>>  3 files changed, 29 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
>>>> index 422033a..246cd9a 100644
>>>> --- a/board/sunxi/Kconfig
>>>> +++ b/board/sunxi/Kconfig
>>>> @@ -223,4 +223,11 @@ config VIDEO
>>>>  	Say Y here to add support for using a cfb console on the HDMI output
>>>>  	found on most sunxi devices.
>>>>  
>>>> +config USB_KEYBOARD
>>>
>>> This seems like it ought to be under drivers/ somewhere, either
>>> drivers/usb/Kconfig or drivers/input/Kconfig perhaps.
>>
>> You're right the problem with that is, that what we really need is
>> to Kconfig-ify the entire USB stuff (eg also CONFIG_USB_EHCI), but that
>> is a bit more then I was planning on working on atm, esp. since that
>> touches many many boards. So this seems best for now.
> 
> OK, is the USB custodian OK with this plan?

Good question, I've just dropped Marek a mail on this with you in the CC.

>>>> +	boolean "Enable USB keyboard support"
>>>> +	default y
>>>> +	---help---
>>>> +	Say Y here to add support for using a USB keyboard (typically used
>>>> +	in combination with a graphical console on HDMI).
>>>> +
>>>>  endif
>>>> diff --git a/configs/Ippo_q8h_v5_defconfig b/configs/Ippo_q8h_v5_defconfig
>>>> index 53df213..50c2f93 100644
>>>> --- a/configs/Ippo_q8h_v5_defconfig
>>>> +++ b/configs/Ippo_q8h_v5_defconfig
>>>> @@ -5,3 +5,4 @@ CONFIG_MACH_SUN8I=y
>>>>  CONFIG_TARGET_IPPO_Q8H_V5=y
>>>>  CONFIG_DEFAULT_DEVICE_TREE="sun8i-a23-ippo-q8h-v5.dtb"
>>>>  CONFIG_VIDEO=n
>>>> +CONFIG_USB_KEYBOARD=n
>>>
>>> Is this the only platform with video+usb which you have? What about e.g.
>>> cubie*, bananapi etc?
>>
>> You mean without video + usb, since the default is y, and it is being
>> forced to n here.
> 
> Sorry, I read this backwards somehow!
> 
>> No this is not the only board we support without HDMI,
>> I has forgotten about the A13 boards (fixed locally now), this is the
>> only board without a host usb connector (it is a tablet), it does support
>> otg, but we do not support that yet.
>>
>> I've also made a local change to disable CONFIG_USB_KEYBOARD by default
>> in the A13 boards since although it does work there it makes little
>> sense to have it when their is no video out support.
> 
> Sounds good.

Ok.

>>>> @@ -298,17 +304,28 @@
>>>>  
>>>>  #include <config_distro_bootcmd.h>
>>>>  
>>>> +#ifdef CONFIG_USB_KEYBOARD
>>>> +#define CONSOLE_IN_SETTINGS \
>>>> +	"preboot=usb start\0" \
>>>> +	"stdin=serial,usbkbd\0"
>>>> +#else
>>>> +#define CONSOLE_IN_SETTINGS \
>>>> +	"stdin=serial\0"
>>>> +#endif
>>>> +
>>>>  #ifdef CONFIG_VIDEO
>>>> -#define CONSOLE_ENV_SETTINGS \
>>>> -	"stdin=serial\0" \
>>>> +#define CONSOLE_OUT_SETTINGS \
>>>>  	"stdout=serial,vga\0" \
>>>>  	"stderr=serial,vga\0"
>>>>  #else
>>>> -#define CONSOLE_ENV_SETTINGS
>>>> +#define CONSOLE_OUT_SETTINGS \
>>>> +	"stdout=serial\0" \
>>>> +	"stderr=serial\0"
>>>
>>> Ah, here are the settings I asked about earlier. Are these in the wrong
>>> patch or did something change in this patch which makes them needed only
>>> now?
>>
>> These are only necessary when CONFIG_SYS_CONSOLE_IS_IN_ENV is set, which
>> sofar was only happening when CONFIG_VIDEO is set. In this patch I'm setting
>> them always to also support just CONFIG_USB_KEYBOARD without CONFIG_VIDEO,
>> which is a bit weird, but I do not want to disallow it.
>>
>> This does mean that we should also unconditionally enable
>> CONFIG_SYS_CONSOLE_IS_IN_ENV with this patch (moving it out of the
>> #ifdef CONFIG_VIDEO) block, which I've not done, I'll fix this locally.
> 
> OK. Not sure what you were planning but it may makes sense to fold some
> portion of that change into the earlier patch.

Yeah, I'll go move things around a bit then post a v2 of the last 4 patches
of this set.

Regards,

Hans

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Ian Campbell]

devicetree@, comments on the requirement that a node have a specific
path welcomed.

On Sun, 2014-11-16 at 14:52 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/16/2014 12:50 PM, Ian Campbell wrote:
> > On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> >> From: Luc Verhaegen <libv@skynet.be>
> >>
> >> Add simplefb support, note this depends on the kernel having support for
> >> the clocks property which has recently been added to the simplefb devicetree
> >> binding.
> > 
> > Link please, Linus's tree[0] doesn't seem to have it yet, I suppose it
> > is in some maintainer tree at the moment? It's not even in linux-next
> > yet though[1]. Maybe simple-framebuffer.txt isn't the right place to
> > look?
> 
> Right now it is sitting here, which is the fbdev maintainers official tree:
> https://git.kernel.org/cgit/linux/kernel/git/tomba/linux.git/log/?h=for-next
> 
> > 
> > [0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> > [1] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> > 
> >> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
> >> +void
> >> +sunxi_simplefb_setup(void *blob)
> >> +{
> >> +	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
> >> +	const char *node = "/chosen/framebuffer0-hdmi";
> >> +	const char *format = "x8r8g8b8";
> > 
> > The bindings doc currently says:
> >         
> >         - format: The format of the framebuffer surface. Valid values are:
> >           - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
> >           - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
> >         
> > Perhaps x8r8g8b8 is defined in the updated version?
> 
> Erm, no, I don't think anyone has actually bothered to keep the list in the
> binding up2date with what the kernel actually supports, x8r8g8b8 has been
> supported in the kernel before sunxi + simplefb every became a topic.

That's a shame, OS authors shouldn't really be expected to scrobble
about in Linux source to figure out what a binding is.

> >> +	const char *okay = "okay";
> >> +	char name[32];
> >> +	fdt32_t cells[2];
> >> +	int offset, stride, ret;
> >> +
> >> +	if (!sunxi_display.enabled)
> >> +		return;
> >> +
> >> +	offset = fdt_path_offset(blob, node);
> > 
> > I think you should use fdt_node_offset_by_compatible here instead of
> > hardcoding a path.
> 
> Hardcoding a path is deliberate. I don't know if you've read the
> previous u-boot code for this, but it did a lot of dt parsing to
> find clocks and add phandles to them, the way we eventually settled
> on when discussing this is for the dts to contain pre-populated simplefb
> nodes which u-boot just needs to fill with the mode info and enable, this
> way as we add support for more clocks (like the module clocks for the
> various display pipeline blocks), we don't need to update u-boot in lock-step,
> we just add the clocks to the simplefb node in the dts file when they get
> added to the dts file in the first place. See the updated bindings.

AFAICT this in no way invalidates what I suggested. There's no reason
why the need to populate/tweak a pre-existing node should have anything
to do with where the node is in the device-tree.

My suggestion literally amounts to:
  - offset = fdt_path_offset(blob, node);
  + offset = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer");

Nothing else in the code changes. You can still add the required details
to the prepopulated node, no matter where it lives.

I notice that v1 of these bindings was posted mid last week and were
still being discussed (at v5) on Friday, where there was active
discussion by DT maintainers (Grant, who hasn't yet acked it AFAICT). So
why is this in the fb tree already?

It seems to me that this binding is not yet at the point where u-boot
should be basing implementation on it. I'm sorry if this means this
feature misses the current u-boot merge window, but there will be
another soon. (didn't this one close on the 8th anyway?)

> > common/lcd.c does it this way too, it also doesn't
> > appear to use a node under /chosen. Perhaps this is changed in the more
> > uptodate binding which I've not seen yet.
> 
> The use of /chosen is part of the updated bindings, which were discussed
> in length on various lists, and then eventually I spend 2 days online with
> Grant Likely in #devicetree to hash things out.
> 
> >> +	cells[0] = cpu_to_fdt32(gd->fb_base);
> >> +	cells[1] = cpu_to_fdt32(CONFIG_SUNXI_FB_SIZE);
> >> +	ret = fdt_setprop(blob, offset, "reg", cells, sizeof(cells[0]) * 2);
> > 
> > What arranges that #address-cells and #size-cells are both 1 at this
> > point?
> 
> The pre-filled simplefb node.

I think you should use fdt_address_cells and fdt_size_cells to ensure
that it really is the case that they are as you expect. Or even better
use them to just DTRT regardless of what the pre-filled node's parent
says.

I'm not really happy with the implication that the DT has to be the
exact one provided in the Linux source tree, and with the level of
coupling which is implied by this patch.

I should be able to write my own DT for a device so long as it follows
the bindings. e.g. if *BSD supports a board (they often seem to have
their own DT files) then I should be able to boot that from u-boot too,
so long as all three follow the bindings.

If the u-boot code is going to put additional constraints on things over
and above the bindings (e.g. requiring that node to be under a
particular #{address/size}-cells regimen and IMHO the specific path
comes under this too despite what recent bindings updates attempt to
say) then that needs to be clearly documented somewhere, and even that
would make me slightly sad given how easy it would be to just make it
work following the bindings in the normal way.

Ian.

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Hans de Goede]

Hi,

On 11/16/2014 03:38 PM, Ian Campbell wrote:
> devicetree@, comments on the requirement that a node have a specific
> path welcomed.
> 
> On Sun, 2014-11-16 at 14:52 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/16/2014 12:50 PM, Ian Campbell wrote:
>>> On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
>>>> From: Luc Verhaegen <libv@skynet.be>
>>>>
>>>> Add simplefb support, note this depends on the kernel having support for
>>>> the clocks property which has recently been added to the simplefb devicetree
>>>> binding.
>>>
>>> Link please, Linus's tree[0] doesn't seem to have it yet, I suppose it
>>> is in some maintainer tree at the moment? It's not even in linux-next
>>> yet though[1]. Maybe simple-framebuffer.txt isn't the right place to
>>> look?
>>
>> Right now it is sitting here, which is the fbdev maintainers official tree:
>> https://git.kernel.org/cgit/linux/kernel/git/tomba/linux.git/log/?h=for-next
>>
>>>
>>> [0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> [1] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>>
>>>> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
>>>> +void
>>>> +sunxi_simplefb_setup(void *blob)
>>>> +{
>>>> +	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
>>>> +	const char *node = "/chosen/framebuffer0-hdmi";
>>>> +	const char *format = "x8r8g8b8";
>>>
>>> The bindings doc currently says:
>>>         
>>>         - format: The format of the framebuffer surface. Valid values are:
>>>           - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>>>           - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>>>         
>>> Perhaps x8r8g8b8 is defined in the updated version?
>>
>> Erm, no, I don't think anyone has actually bothered to keep the list in the
>> binding up2date with what the kernel actually supports, x8r8g8b8 has been
>> supported in the kernel before sunxi + simplefb every became a topic.
> 
> That's a shame, OS authors shouldn't really be expected to scrobble
> about in Linux source to figure out what a binding is.
> 
>>>> +	const char *okay = "okay";
>>>> +	char name[32];
>>>> +	fdt32_t cells[2];
>>>> +	int offset, stride, ret;
>>>> +
>>>> +	if (!sunxi_display.enabled)
>>>> +		return;
>>>> +
>>>> +	offset = fdt_path_offset(blob, node);
>>>
>>> I think you should use fdt_node_offset_by_compatible here instead of
>>> hardcoding a path.
>>
>> Hardcoding a path is deliberate. I don't know if you've read the
>> previous u-boot code for this, but it did a lot of dt parsing to
>> find clocks and add phandles to them, the way we eventually settled
>> on when discussing this is for the dts to contain pre-populated simplefb
>> nodes which u-boot just needs to fill with the mode info and enable, this
>> way as we add support for more clocks (like the module clocks for the
>> various display pipeline blocks), we don't need to update u-boot in lock-step,
>> we just add the clocks to the simplefb node in the dts file when they get
>> added to the dts file in the first place. See the updated bindings.
> 
> AFAICT this in no way invalidates what I suggested. There's no reason
> why the need to populate/tweak a pre-existing node should have anything
> to do with where the node is in the device-tree.

Right, I forgot to add one important bit to my explanation, sorry, if
you look at the binding then it says that the name should be suffixed
with the output type, so currently the sunxi u-boot code will look for
/chosen/framebuffer0-hdmi. Which will e.g. also happen on any A10
based tablets which typically have both hdmi out and an lcd screen,
in the future I hope we will also get lcd support in u-boot, and then
logically on tablets the lcd screen would take precedence, so then
unless overriden through some config mechanism u-boot will chose
to use the lcd, and will look for /chosen/framebuffer0-lcd which has
a different set of clocks then /chosen/framebuffer0-hdmi.

> 
> My suggestion literally amounts to:
>   - offset = fdt_path_offset(blob, node);
>   + offset = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer");
> 
> Nothing else in the code changes. You can still add the required details
> to the prepopulated node, no matter where it lives.

See above, we need to pick the right pre-populated node for the output
type, this esp. becomes important on boxes like the mele a1000 and friends
where there are 3 outputs to divide over 2 display pipelines (so we can
only lit up 2 of the outputs).

> I notice that v1 of these bindings was posted mid last week and were
> still being discussed (at v5) on Friday, where there was active
> discussion by DT maintainers (Grant, who hasn't yet acked it AFAICT). So
> why is this in the fb tree already?

Grant only had some minor tweaks for v4, so Tomi has merged it based on
that, with a request to do any fixups as follow up patches.

As for the timeline, even though some of the new binding stuff was only
finalized last week, the whole discussion about this has been going one for
months.

> It seems to me that this binding is not yet at the point where u-boot
> should be basing implementation on it. I'm sorry if this means this
> feature misses the current u-boot merge window, but there will be
> another soon. (didn't this one close on the 8th anyway?)

The devicetree bindings have been going on for so long, that this would
be the 2nd merge window this feature misses, Luc's original version was
posted before the v2014.10 merge window.

AFAIK Grant agrees with v5 as merged by Timo, and it would be really nice
to move forward with this rather then to start yet another round of
never ending discussions on this.

>>> common/lcd.c does it this way too, it also doesn't
>>> appear to use a node under /chosen. Perhaps this is changed in the more
>>> uptodate binding which I've not seen yet.
>>
>> The use of /chosen is part of the updated bindings, which were discussed
>> in length on various lists, and then eventually I spend 2 days online with
>> Grant Likely in #devicetree to hash things out.
>>
>>>> +	cells[0] = cpu_to_fdt32(gd->fb_base);
>>>> +	cells[1] = cpu_to_fdt32(CONFIG_SUNXI_FB_SIZE);
>>>> +	ret = fdt_setprop(blob, offset, "reg", cells, sizeof(cells[0]) * 2);
>>>
>>> What arranges that #address-cells and #size-cells are both 1 at this
>>> point?
>>
>> The pre-filled simplefb node.
> 
> I think you should use fdt_address_cells and fdt_size_cells to ensure
> that it really is the case that they are as you expect. Or even better
> use them to just DTRT regardless of what the pre-filled node's parent
> says.

Verifying things to be as expected seem sensible, DTRT regardless seems
impossible, since u-boot will have no idea what needs to be in there
if either size-cells or address-cells != 1.

> I'm not really happy with the implication that the DT has to be the
> exact one provided in the Linux source tree, and with the level of
> coupling which is implied by this patch.

Some level of coupling is always needed that is what the bindings docs
are for. Specifying how the bindings should look like from a firmware pov
is no different then specifying how they should look like from an os pov.

> I should be able to write my own DT for a device so long as it follows
> the bindings. e.g. if *BSD supports a board (they often seem to have
> their own DT files) then I should be able to boot that from u-boot too,
> so long as all three follow the bindings.

Ack, and I don't see how what we're doing here breaks that.

> If the u-boot code is going to put additional constraints on things over
> and above the bindings (e.g. requiring that node to be under a
> particular #{address/size}-cells regimen

Bindings always specify a particular address / size cells regimen, otherwise
it is not well defined how to interpret them. If there are extra address /
size fields those need to have a defined meaning, so an #{address/size}-cells regimen
is always implied by the bindings.

In this case the address / size cells comes from how we specify any memory
address on sunxi which is 1 / 1.

> and IMHO the specific path
> comes under this too despite what recent bindings updates attempt to
> say) then that needs to be clearly documented somewhere, and even that
> would make me slightly sad given how easy it would be to just make it
> work following the bindings in the normal way.

The path requirement comes from 2 things:

1) u-boot needs to be able to find which node belongs to which display pipeline
2) u-boot needs to be able to find the right node for the output chosen, were
there may be multiple output options u-boot can chose for a single display
pipeline

So just searching for the node by compatible will not work.

Regards,

Hans

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Ian Campbell]

On Sun, 2014-11-16 at 16:11 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/16/2014 03:38 PM, Ian Campbell wrote:
> > devicetree@, comments on the requirement that a node have a specific
> > path welcomed.
> > 
> > On Sun, 2014-11-16 at 14:52 +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 11/16/2014 12:50 PM, Ian Campbell wrote:
> >>> On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> >>>> From: Luc Verhaegen <libv@skynet.be>
> >>>>
> >>>> Add simplefb support, note this depends on the kernel having support for
> >>>> the clocks property which has recently been added to the simplefb devicetree
> >>>> binding.
> >>>
> >>> Link please, Linus's tree[0] doesn't seem to have it yet, I suppose it
> >>> is in some maintainer tree at the moment? It's not even in linux-next
> >>> yet though[1]. Maybe simple-framebuffer.txt isn't the right place to
> >>> look?
> >>
> >> Right now it is sitting here, which is the fbdev maintainers official tree:
> >> https://git.kernel.org/cgit/linux/kernel/git/tomba/linux.git/log/?h=for-next
> >>
> >>>
> >>> [0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> >>> [1] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> >>>
> >>>> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
> >>>> +void
> >>>> +sunxi_simplefb_setup(void *blob)
> >>>> +{
> >>>> +	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
> >>>> +	const char *node = "/chosen/framebuffer0-hdmi";
> >>>> +	const char *format = "x8r8g8b8";
> >>>
> >>> The bindings doc currently says:
> >>>         
> >>>         - format: The format of the framebuffer surface. Valid values are:
> >>>           - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
> >>>           - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
> >>>         
> >>> Perhaps x8r8g8b8 is defined in the updated version?
> >>
> >> Erm, no, I don't think anyone has actually bothered to keep the list in the
> >> binding up2date with what the kernel actually supports, x8r8g8b8 has been
> >> supported in the kernel before sunxi + simplefb every became a topic.
> > 
> > That's a shame, OS authors shouldn't really be expected to scrobble
> > about in Linux source to figure out what a binding is.
> > 
> >>>> +	const char *okay = "okay";
> >>>> +	char name[32];
> >>>> +	fdt32_t cells[2];
> >>>> +	int offset, stride, ret;
> >>>> +
> >>>> +	if (!sunxi_display.enabled)
> >>>> +		return;
> >>>> +
> >>>> +	offset = fdt_path_offset(blob, node);
> >>>
> >>> I think you should use fdt_node_offset_by_compatible here instead of
> >>> hardcoding a path.
> >>
> >> Hardcoding a path is deliberate. I don't know if you've read the
> >> previous u-boot code for this, but it did a lot of dt parsing to
> >> find clocks and add phandles to them, the way we eventually settled
> >> on when discussing this is for the dts to contain pre-populated simplefb
> >> nodes which u-boot just needs to fill with the mode info and enable, this
> >> way as we add support for more clocks (like the module clocks for the
> >> various display pipeline blocks), we don't need to update u-boot in lock-step,
> >> we just add the clocks to the simplefb node in the dts file when they get
> >> added to the dts file in the first place. See the updated bindings.
> > 
> > AFAICT this in no way invalidates what I suggested. There's no reason
> > why the need to populate/tweak a pre-existing node should have anything
> > to do with where the node is in the device-tree.
> 
> Right, I forgot to add one important bit to my explanation, sorry, if
> you look at the binding then it says that the name should be suffixed
> with the output type, so currently the sunxi u-boot code will look for
> /chosen/framebuffer0-hdmi. Which will e.g. also happen on any A10
> based tablets which typically have both hdmi out and an lcd screen,
> in the future I hope we will also get lcd support in u-boot, and then
> logically on tablets the lcd screen would take precedence, so then
> unless overriden through some config mechanism u-boot will chose
> to use the lcd, and will look for /chosen/framebuffer0-lcd which has
> a different set of clocks then /chosen/framebuffer0-hdmi.

This sounds like a use for:
        compatible = "simple-framebuffer,hdmi", "simple-framebuffer"
or some such, that's almost exactly what multiople compatible strings
are for. Relying on specific naming and/or path is rather
unconventional.

> >>> common/lcd.c does it this way too, it also doesn't
> >>> appear to use a node under /chosen. Perhaps this is changed in the more
> >>> uptodate binding which I've not seen yet.
> >>
> >> The use of /chosen is part of the updated bindings, which were discussed
> >> in length on various lists, and then eventually I spend 2 days online with
> >> Grant Likely in #devicetree to hash things out.
> >>
> >>>> +	cells[0] = cpu_to_fdt32(gd->fb_base);
> >>>> +	cells[1] = cpu_to_fdt32(CONFIG_SUNXI_FB_SIZE);
> >>>> +	ret = fdt_setprop(blob, offset, "reg", cells, sizeof(cells[0]) * 2);
> >>>
> >>> What arranges that #address-cells and #size-cells are both 1 at this
> >>> point?
> >>
> >> The pre-filled simplefb node.
> > 
> > I think you should use fdt_address_cells and fdt_size_cells to ensure
> > that it really is the case that they are as you expect. Or even better
> > use them to just DTRT regardless of what the pre-filled node's parent
> > says.
> 
> Verifying things to be as expected seem sensible, DTRT regardless seems
> impossible, since u-boot will have no idea what needs to be in there
> if either size-cells or address-cells != 1.

I think you have misunderstood what #size-cells / #address-cells are.
They tell you the number of cells which each address/size entry uses,
they do not tell you anything about the number of address/size
combinations in the reg property (i.e. the number of registers, which is
indeed a property of the binding).

IOW for a single region at 0xabcdef12 size 0x00003456 then:

#address-cells=1,#size-cells=1 => regs = <0xabcdef12 0x00003456>
#address-cells=2,#size-cells=1 => regs = <0 0xabcdef12 0x00003456>
#address-cells=1,#size-cells=2 => regs = <0xabcdef12 0 0x00003456>
#address-cells=2,#size-cells=1 => regs = <0 0xabcdef12 0 0x00003456>

The difference is between between storing the number as a u32 or a u64
(encoded as two cells), it doesn't change the meaning of anything.

> > I'm not really happy with the implication that the DT has to be the
> > exact one provided in the Linux source tree, and with the level of
> > coupling which is implied by this patch.
> 
> Some level of coupling is always needed that is what the bindings docs
> are for. Specifying how the bindings should look like from a firmware pov
> is no different then specifying how they should look like from an os pov.

This code is making assumptions over and above the bindings, and the
bindings themselves do not seem to be fully agreed upon yet (and contain
some unconventional constructs).

> The devicetree bindings have been going on for so long, that this
> would be the 2nd merge window this feature misses, Luc's original
> version was posted before the v2014.10 merge window.

I'm afraid I don't agree that just because it has taken a long time to
get something right we should commit to it before it is ready,
especially when it is an ABI, which is what a DT binding is.

If this scheme was acked by a DT maintainer then I'd be fine with it,
but so far it hasn't been, at least not publicly in the threads I've
looked at.

> AFAIK Grant agrees with v5

AFAIK Grant hasn't actually said that. If he does ack it (or if someone
points me to the correct mail) then I have no further objections.

> > I should be able to write my own DT for a device so long as it follows
> > the bindings. e.g. if *BSD supports a board (they often seem to have
> > their own DT files) then I should be able to boot that from u-boot too,
> > so long as all three follow the bindings.
> 
> Ack, and I don't see how what we're doing here breaks that.

An author would need to know this out of band requirement that #*-cells
are required to be 1, which is an unusual requirement to start with.

> > If the u-boot code is going to put additional constraints on things over
> > and above the bindings (e.g. requiring that node to be under a
> > particular #{address/size}-cells regimen
> 
> Bindings always specify a particular address / size cells regimen, otherwise
> it is not well defined how to interpret them.

No so. Almost no bindings specify these, they are a common device-tree
concept and for a given node are defined by that node's parent (or
grandparent, etc), not by the node's bindings. This is all part of the
way buses are represented in DT. 

In fact it's a bit odd to have a reg property under /chosen at all,
since it doesn't really fit in with the bus structure. I've done
something similar in some bindings I've authored[0], but AIUI I got that
wrong and really should have used a set of non-reg properties with a
single value so there was no need to parse using #*-cells  (cf the
initrd-start + initrd-end properties under /chosen). Sadly DT is an ABI,
so for my bindings I'm kind of stuck with it for the foreseeable future.

[0]
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=08ed7751859dbe2d2c32d86d7df371057d7b45a4;hb=HEAD

>  If there are extra address /
> size fields those need to have a defined meaning, so an #{address/size}-cells regimen
> is always implied by the bindings.
> 
> In this case the address / size cells comes from how we specify any memory
> address on sunxi which is 1 / 1.

That just happens to be what the dt's in the Linux source have written
into them, there is nothing fundamental which requires this and you
could perfectly validly author a sunxi DT which uses 2/2 or 2/1 etc.

> > and IMHO the specific path
> > comes under this too despite what recent bindings updates attempt to
> > say) then that needs to be clearly documented somewhere, and even that
> > would make me slightly sad given how easy it would be to just make it
> > work following the bindings in the normal way.
> 
> The path requirement comes from 2 things:
> 
> 1) u-boot needs to be able to find which node belongs to which display pipeline
> 2) u-boot needs to be able to find the right node for the output chosen, were
> there may be multiple output options u-boot can chose for a single display
> pipeline
> 
> So just searching for the node by compatible will not work.

It would work if you used compatible how it was intended to be used.

Ian.

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Ian Campbell]

On Sun, 2014-11-16 at 16:11 +0000, Ian Campbell wrote:
> 
> > AFAIK Grant agrees with v5
> 
> AFAIK Grant hasn't actually said that. If he does ack it (or if
> someone
> points me to the correct mail) then I have no further objections.

I finally found
<CACxGe6ssdeOnQnem1012h_jFK1wWOD_WwosjFgiAcgBoqyDt9A@mail.gmail.com>
which for some reason isn't in my devicetree@ folder nor in any of the
archives online I looked at earlier (I could only find it by message-id
now I know it), but it was in my sunxi folder.

So with Grant and Rob having accepted them you can consider my concerns
with the bindings alleviated.

Ian.

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Hans de Goede]

Hi,

On 11/16/2014 06:19 PM, Ian Campbell wrote:
> On Sun, 2014-11-16 at 16:11 +0000, Ian Campbell wrote:
>>
>>> AFAIK Grant agrees with v5
>>
>> AFAIK Grant hasn't actually said that. If he does ack it (or if
>> someone
>> points me to the correct mail) then I have no further objections.
> 
> I finally found
> <CACxGe6ssdeOnQnem1012h_jFK1wWOD_WwosjFgiAcgBoqyDt9A@mail.gmail.com>
> which for some reason isn't in my devicetree@ folder nor in any of the
> archives online I looked at earlier (I could only find it by message-id
> now I know it), but it was in my sunxi folder.
> 
> So with Grant and Rob having accepted them you can consider my concerns
> with the bindings alleviated.

Thanks! I was about to reply to your earlier mail with lets wait for
Grant to get online, and see what he has to say, but this works too.

And thanks for explaining the address and size cells thing better, I
did know that for address and size #cells determines u32 vs u64, I'm
used to gpio-s / clock-s where extra cells are an extra parameter,
hence my confusion.

I'll do a v3 of the simplefb patch which takes the address and size
#cells into account.

Regards,

Hans

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Grant Likely]

On Sun, Nov 16, 2014 at 4:11 PM, Ian Campbell <ijc@hellion.org.uk> wrote:
> On Sun, 2014-11-16 at 16:11 +0100, Hans de Goede wrote:
>> On 11/16/2014 03:38 PM, Ian Campbell wrote:
>> > On Sun, 2014-11-16 at 14:52 +0100, Hans de Goede wrote:
>> >> Hardcoding a path is deliberate. I don't know if you've read the
>> >> previous u-boot code for this, but it did a lot of dt parsing to
>> >> find clocks and add phandles to them, the way we eventually settled
>> >> on when discussing this is for the dts to contain pre-populated simplefb
>> >> nodes which u-boot just needs to fill with the mode info and enable, this
>> >> way as we add support for more clocks (like the module clocks for the
>> >> various display pipeline blocks), we don't need to update u-boot in lock-step,
>> >> we just add the clocks to the simplefb node in the dts file when they get
>> >> added to the dts file in the first place. See the updated bindings.
>> >
>> > AFAICT this in no way invalidates what I suggested. There's no reason
>> > why the need to populate/tweak a pre-existing node should have anything
>> > to do with where the node is in the device-tree.
>>
>> Right, I forgot to add one important bit to my explanation, sorry, if
>> you look at the binding then it says that the name should be suffixed
>> with the output type, so currently the sunxi u-boot code will look for
>> /chosen/framebuffer0-hdmi. Which will e.g. also happen on any A10
>> based tablets which typically have both hdmi out and an lcd screen,
>> in the future I hope we will also get lcd support in u-boot, and then
>> logically on tablets the lcd screen would take precedence, so then
>> unless overriden through some config mechanism u-boot will chose
>> to use the lcd, and will look for /chosen/framebuffer0-lcd which has
>> a different set of clocks then /chosen/framebuffer0-hdmi.
>
> This sounds like a use for:
>         compatible = "simple-framebuffer,hdmi", "simple-framebuffer"
> or some such, that's almost exactly what multiople compatible strings
> are for. Relying on specific naming and/or path is rather
> unconventional.

Indeed, for a long time now finding nodes by path has been strongly
discouraged. It makes it hard to move nodes around when needed. I'm
not going to make a big deal about it because it doesn't affect the OS
interface, but Ian's suggestion is sane. simple-framebuffer is enough
to identify the binding, but you can use additional strings to
identify the specific hardware interface that U-Boot can use to locate
the node. ie. sunxi,framebuffer-hdml or some such. You can never be
sure when someone will produce a board that messes with your naming
assumptions. What if is suddenly needs to be named "framebuffer1-hdmi"
because they added and FPGA that they want as framebuffer0? (Yes, I'm
making stuff up, but I have to think about the corner cases)

>> The devicetree bindings have been going on for so long, that this
>> would be the 2nd merge window this feature misses, Luc's original
>> version was posted before the v2014.10 merge window.
>
> I'm afraid I don't agree that just because it has taken a long time to
> get something right we should commit to it before it is ready,
> especially when it is an ABI, which is what a DT binding is.
>
> If this scheme was acked by a DT maintainer then I'd be fine with it,
> but so far it hasn't been, at least not publicly in the threads I've
> looked at.

I /DO/ want comments though. Putting the node in /chosen is
unconventional. I want to hear if anyone has a good reason why the
framebuffers shouldn't be placed into /chosen.

>> AFAIK Grant agrees with v5
>
> AFAIK Grant hasn't actually said that. If he does ack it (or if someone
> points me to the correct mail) then I have no further objections.

My word also isn't gospel. On controversial stuff I want to have
consensus. For the clock patches and had a long conversation with Rob
to make sure we were both in agreement before giving my final ack.

> In fact it's a bit odd to have a reg property under /chosen at all,
> since it doesn't really fit in with the bus structure. I've done
> something similar in some bindings I've authored[0], but AIUI I got that
> wrong and really should have used a set of non-reg properties with a
> single value so there was no need to parse using #*-cells  (cf the
> initrd-start + initrd-end properties under /chosen). Sadly DT is an ABI,
> so for my bindings I'm kind of stuck with it for the foreseeable future.
>
> [0]
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=08ed7751859dbe2d2c32d86d7df371057d7b45a4;hb=HEAD

Ironic isn't it that I though of that as precedence when I suggested
/chosen! :-)

I actually don't have a problem with it. We do need a way to specify
runtime memory usage, and /chosen is as good a place as any,
particularly when it represents things that won't necessarily be
relevant on kexec or dom0 boot.

The other options are under either the /memory or the /reserved-memory
tree. Rob and I talked about /reserved-memory quite a lot. We could
put all the framebuffer details into the memory node that reserves the
framebuffer. However, I don't like that idea because it kind of makes
assumptions about how the framebuffer will be located inside the
memory region and doesn't allow for multiple framebuffers within a
single region.

g.

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Hans de Goede]

Hi,

On 11/17/2014 10:58 AM, Grant Likely wrote:
> On Sun, Nov 16, 2014 at 4:11 PM, Ian Campbell <ijc@hellion.org.uk> wrote:
>> On Sun, 2014-11-16 at 16:11 +0100, Hans de Goede wrote:

<snip>

>>> Right, I forgot to add one important bit to my explanation, sorry, if
>>> you look at the binding then it says that the name should be suffixed
>>> with the output type, so currently the sunxi u-boot code will look for
>>> /chosen/framebuffer0-hdmi. Which will e.g. also happen on any A10
>>> based tablets which typically have both hdmi out and an lcd screen,
>>> in the future I hope we will also get lcd support in u-boot, and then
>>> logically on tablets the lcd screen would take precedence, so then
>>> unless overriden through some config mechanism u-boot will chose
>>> to use the lcd, and will look for /chosen/framebuffer0-lcd which has
>>> a different set of clocks then /chosen/framebuffer0-hdmi.
>>
>> This sounds like a use for:
>>         compatible = "simple-framebuffer,hdmi", "simple-framebuffer"
>> or some such, that's almost exactly what multiople compatible strings
>> are for. Relying on specific naming and/or path is rather
>> unconventional.
> 
> Indeed, for a long time now finding nodes by path has been strongly
> discouraged. It makes it hard to move nodes around when needed. I'm
> not going to make a big deal about it because it doesn't affect the OS
> interface, but Ian's suggestion is sane. simple-framebuffer is enough
> to identify the binding, but you can use additional strings to
> identify the specific hardware interface that U-Boot can use to locate
> the node. ie. sunxi,framebuffer-hdml or some such. You can never be
> sure when someone will produce a board that messes with your naming
> assumptions. What if is suddenly needs to be named "framebuffer1-hdmi"
> because they added and FPGA that they want as framebuffer0? (Yes, I'm
> making stuff up, but I have to think about the corner cases)

I like the suggestion of using a compatible of : "sunxi,framebuffer-hdmi"
although it will need to be : "sunxi,framebuffer0-hdmi" as u-boot
needs to know which simplefb node to use for which display pipeline
(so that the node has the right clocks and whats not).

And I think it makes sense to put simple in there as it is a pre-populated
simplefb node , so then we get:

"sunxi,simple-framebuffer0-hdmi"

And we can drop the unconventional lookup by path.

I'll go on irc now, and join #devicetree, Ian, maybe you can join us
there to and we can hash out something we can all agree on ?

And then I'll do yet another set of patches (to apply on top on the
kernel side, and a new version of the u-boot patch), and we'll hopefully
end up with something which makes us all happy

<snip>

>> In fact it's a bit odd to have a reg property under /chosen at all,
>> since it doesn't really fit in with the bus structure. I've done
>> something similar in some bindings I've authored[0], but AIUI I got that
>> wrong and really should have used a set of non-reg properties with a
>> single value so there was no need to parse using #*-cells  (cf the
>> initrd-start + initrd-end properties under /chosen). Sadly DT is an ABI,
>> so for my bindings I'm kind of stuck with it for the foreseeable future.
>>
>> [0]
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=08ed7751859dbe2d2c32d86d7df371057d7b45a4;hb=HEAD
> 
> Ironic isn't it that I though of that as precedence when I suggested
> /chosen! :-)
> 
> I actually don't have a problem with it. We do need a way to specify
> runtime memory usage, and /chosen is as good a place as any,
> particularly when it represents things that won't necessarily be
> relevant on kexec or dom0 boot.
> 
> The other options are under either the /memory or the /reserved-memory
> tree. Rob and I talked about /reserved-memory quite a lot. We could
> put all the framebuffer details into the memory node that reserves the
> framebuffer. However, I don't like that idea because it kind of makes
> assumptions about how the framebuffer will be located inside the
> memory region and doesn't allow for multiple framebuffers within a
> single region.

I'm not a devicetree expert, but /chosen already came to my mind even
before Grant suggested it, /chosen seems the right place for this to me.

Note that once we drop the path based lookup the location can always be
changed later (to some degree, it still needs to be in a place where
the kernel will bind to it).

Regards,

Hans

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Ian Campbell]

On Mon, 2014-11-17 at 09:58 +0000, Grant Likely wrote:
> I /DO/ want comments though. Putting the node in /chosen is
> unconventional. I want to hear if anyone has a good reason why the
> framebuffers shouldn't be placed into /chosen.

I don't think putting it under /chosen is a problem at all. THe
semantics of some of hte convention properties are a bit odd under
there, but that's not insurmountable.

> >> AFAIK Grant agrees with v5
> >
> > AFAIK Grant hasn't actually said that. If he does ack it (or if someone
> > points me to the correct mail) then I have no further objections.
> 
> My word also isn't gospel.

I suppose I should have said something like "I trust Grant's judgement
more than my own on things relating to DT" ;-).

>  On controversial stuff I want to have
> consensus. For the clock patches and had a long conversation with Rob
> to make sure we were both in agreement before giving my final ack.
> 
> > In fact it's a bit odd to have a reg property under /chosen at all,
> > since it doesn't really fit in with the bus structure. I've done
> > something similar in some bindings I've authored[0], but AIUI I got that
> > wrong and really should have used a set of non-reg properties with a
> > single value so there was no need to parse using #*-cells  (cf the
> > initrd-start + initrd-end properties under /chosen). Sadly DT is an ABI,
> > so for my bindings I'm kind of stuck with it for the foreseeable future.
> >
> > [0]
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=08ed7751859dbe2d2c32d86d7df371057d7b45a4;hb=HEAD
> 
> Ironic isn't it that I though of that as precedence when I suggested
> /chosen! :-)

:-)

> I actually don't have a problem with it. We do need a way to specify
> runtime memory usage, and /chosen is as good a place as any,
> particularly when it represents things that won't necessarily be
> relevant on kexec or dom0 boot.

The main issue which was explained to me with my Xen bindings was that
reg = <> isn't all that meaningful under /chosen because it doesn't fit
into the bus structure, so the #address-/size-cells stuff gets a bit
strange. It's probably tolerable for things which are strictly physical
RAM addresses (as opposed to mmio) since RAM isn't typically behind a
visible bus.

The scheme used for initrds sidesteps all those issues by using separate
(multicellular) properties for the start and end regions and not using
reg=<> and therefore naturally breaking the expected semantic link with
bus topology which reg implies etc.

> The other options are under either the /memory or the /reserved-memory
> tree. Rob and I talked about /reserved-memory quite a lot. We could
> put all the framebuffer details into the memory node that reserves the
> framebuffer. However, I don't like that idea because it kind of makes
> assumptions about how the framebuffer will be located inside the
> memory region and doesn't allow for multiple framebuffers within a
> single region.

Yes, that sounds strictly worse than the current solution to me.

Ian.

[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Grant Likely]

On Mon, Nov 17, 2014 at 10:14 AM, Ian Campbell <ijc@hellion.org.uk> wrote:
> On Mon, 2014-11-17 at 09:58 +0000, Grant Likely wrote:
>> I /DO/ want comments though. Putting the node in /chosen is
>> unconventional. I want to hear if anyone has a good reason why the
>> framebuffers shouldn't be placed into /chosen.
>
> I don't think putting it under /chosen is a problem at all. THe
> semantics of some of hte convention properties are a bit odd under
> there, but that's not insurmountable.
>
>> >> AFAIK Grant agrees with v5
>> >
>> > AFAIK Grant hasn't actually said that. If he does ack it (or if someone
>> > points me to the correct mail) then I have no further objections.
>>
>> My word also isn't gospel.
>
> I suppose I should have said something like "I trust Grant's judgement
> more than my own on things relating to DT" ;-).
>
>>  On controversial stuff I want to have
>> consensus. For the clock patches and had a long conversation with Rob
>> to make sure we were both in agreement before giving my final ack.
>>
>> > In fact it's a bit odd to have a reg property under /chosen at all,
>> > since it doesn't really fit in with the bus structure. I've done
>> > something similar in some bindings I've authored[0], but AIUI I got that
>> > wrong and really should have used a set of non-reg properties with a
>> > single value so there was no need to parse using #*-cells  (cf the
>> > initrd-start + initrd-end properties under /chosen). Sadly DT is an ABI,
>> > so for my bindings I'm kind of stuck with it for the foreseeable future.
>> >
>> > [0]
>> > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=08ed7751859dbe2d2c32d86d7df371057d7b45a4;hb=HEAD
>>
>> Ironic isn't it that I though of that as precedence when I suggested
>> /chosen! :-)
>
> :-)
>
>> I actually don't have a problem with it. We do need a way to specify
>> runtime memory usage, and /chosen is as good a place as any,
>> particularly when it represents things that won't necessarily be
>> relevant on kexec or dom0 boot.
>
> The main issue which was explained to me with my Xen bindings was that
> reg = <> isn't all that meaningful under /chosen because it doesn't fit
> into the bus structure, so the #address-/size-cells stuff gets a bit
> strange. It's probably tolerable for things which are strictly physical
> RAM addresses (as opposed to mmio) since RAM isn't typically behind a
> visible bus.
>
> The scheme used for initrds sidesteps all those issues by using separate
> (multicellular) properties for the start and end regions and not using
> reg=<> and therefore naturally breaking the expected semantic link with
> bus topology which reg implies etc.

I quite dislike the initrd multi-property approach. It has to be
parsed in a completely separate way.

>> The other options are under either the /memory or the /reserved-memory
>> tree. Rob and I talked about /reserved-memory quite a lot. We could
>> put all the framebuffer details into the memory node that reserves the
>> framebuffer. However, I don't like that idea because it kind of makes
>> assumptions about how the framebuffer will be located inside the
>> memory region and doesn't allow for multiple framebuffers within a
>> single region.
>
> Yes, that sounds strictly worse than the current solution to me.

Besides, the simple-framebuffer binding could easily be extended to
allow a reserved-memory reference instead of a reg property if/when
that becomes a better option for a specific platform.

g.