sunxi: HDMI (cfb) console support

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 <[email protected]>

---
 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(-)

arch/arm/cpu/armv7/sunxi/dram_sun4i.c +2 -2

index dc9fdb9..ec8aaa7 100644
@@ -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
 }
 

arch/arm/include/asm/arch-sunxi/clock_sun4i.h +1 -1

index 9dca800..6c0430c 100644
@@ -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 */

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

Signed-off-by Hans de Goede <[email protected]>

---
 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(-)

arch/arm/cpu/armv7/sunxi/clock_sun4i.c +27 -0

index a0e49d1..49f4032 100644
@@ -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);
+}

arch/arm/cpu/armv7/sunxi/clock_sun6i.c +29 -0

index 16ab6f3..8e949c6 100644
@@ -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);
+}

arch/arm/include/asm/arch-sunxi/clock.h +2 -0

index b40c16b..64acff3 100644
@@ -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

arch/arm/include/asm/arch-sunxi/clock_sun4i.h +45 -0

index 6c0430c..eb88969 100644
@@ -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 */

arch/arm/include/asm/arch-sunxi/clock_sun6i.h +58 -1

index e16a764..50a4b69 100644
@@ -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 */

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

On Fri, 14 Nov 2014 21:17:39 +0100
Anatolij Gustschin <[email protected]> wrote:
...

Quoted 1 line

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 <[email protected]>

include/configs/sunxi-common.h line 299

Quoted 10 lines · Hans de Goede

include/configs/sunxi-common.h +9 -0

+/* 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?

include/configs/sunxi-common.h line 299

Quoted 10 lines · Hans de Goede

include/configs/sunxi-common.h +9 -0

+/* 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?

Hi,

Thanks for the reviews!

Quoted 14 lines · Ian Campbell

include/configs/sunxi-common.h line 299

Quoted 10 lines · Hans de Goede

include/configs/sunxi-common.h +9 -0

+/* 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:

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.

Quoted 2 lines

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).

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/video/sunxi_display.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

include/configs/sunxi-common.h +3 -0

index 532fdb7..d7d8571 100644
@@ -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

On Fri, 14 Nov 2014 17:54:47 +0100
Hans de Goede <[email protected]> wrote:
...

Quoted 18 lines

include/configs/sunxi-common.h +3 -0

index 532fdb7..d7d8571 100644
@@ -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)
 

include/configs/sunxi-common.h line 228

Quoted 4 lines

include/configs/sunxi-common.h +4 -0

+/* 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.

From: Luc Verhaegen <[email protected]>

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.

Quoted 5 lines · Hans de Goede

From: Luc Verhaegen <[email protected]>

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

drivers/video/sunxi_display.c video_hw_init(void) · line 430

Quoted 7 lines

drivers/video/sunxi_display.c +7 -0

+#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?

drivers/video/sunxi_display.c video_hw_init(void) · line 439

Quoted 10 lines

drivers/video/sunxi_display.c +9 -0

+	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.

drivers/video/sunxi_display.c video_hw_init(void) · line 452

Quoted 3 lines

drivers/video/sunxi_display.c +3 -0

+	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.

drivers/video/sunxi_display.c video_hw_init(void) · line 476

Quoted 6 lines

drivers/video/sunxi_display.c +5 -0

+	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.

Hi,

Quoted 11 lines · Ian Campbell

Quoted 5 lines · Hans de Goede

From: Luc Verhaegen <[email protected]>

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

Quoted 18 lines

[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

drivers/video/sunxi_display.c video_hw_init(void) · line 430

Quoted 7 lines

drivers/video/sunxi_display.c +7 -0

+#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).

Quoted 1 line

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.

Quoted 4 lines

Quoted 4 lines

drivers/video/sunxi_display.c +4 -0

+	const char *okay = "okay";
+	char name[32];
+	fdt32_t cells[2];
+	int offset, stride, ret;

Quoted 8 lines

drivers/video/sunxi_display.c video_hw_init(void) · line 439

Quoted 5 lines

drivers/video/sunxi_display.c +5 -0

+
+	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.

Quoted 3 lines

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.

Quoted 6 lines

drivers/video/sunxi_display.c video_hw_init(void) · line 452

Quoted 3 lines

drivers/video/sunxi_display.c +3 -0

+	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.

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

Quoted 36 lines · Hans de Goede

Hi,

Quoted 11 lines · Ian Campbell

Quoted 5 lines · Hans de Goede

From: Luc Verhaegen <[email protected]>

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

Quoted 18 lines

[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

drivers/video/sunxi_display.c video_hw_init(void) · line 430

Quoted 7 lines

drivers/video/sunxi_display.c +7 -0

+#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).

Quoted 5 lines

Quoted 1 line

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.

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

drivers/video/sunxi_display.c +4 -0

+	const char *okay = "okay";
+	char name[32];
+	fdt32_t cells[2];
+	int offset, stride, ret;

Quoted 8 lines

drivers/video/sunxi_display.c video_hw_init(void) · line 439

Quoted 5 lines

drivers/video/sunxi_display.c +5 -0

+
+	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.

Quoted 9 lines

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?)

Quoted 15 lines

Quoted 3 lines

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.

Quoted 6 lines

drivers/video/sunxi_display.c video_hw_init(void) · line 452

Quoted 3 lines

drivers/video/sunxi_display.c +3 -0

+	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?

Quoted 1 line

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.

Hi,

Quoted 72 lines · Ian Campbell

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

Quoted 36 lines · Hans de Goede

Hi,

Quoted 11 lines · Ian Campbell

Quoted 5 lines · Hans de Goede

From: Luc Verhaegen <[email protected]>

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

Quoted 18 lines

[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

drivers/video/sunxi_display.c video_hw_init(void) · line 430

Quoted 7 lines

drivers/video/sunxi_display.c +7 -0

+#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).

Quoted 5 lines

Quoted 1 line

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.

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

drivers/video/sunxi_display.c +4 -0

+	const char *okay = "okay";
+	char name[32];
+	fdt32_t cells[2];
+	int offset, stride, ret;

Quoted 8 lines

drivers/video/sunxi_display.c video_hw_init(void) · line 439

Quoted 5 lines

drivers/video/sunxi_display.c +5 -0

+
+	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.

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 17 lines

Quoted 16 lines

Quoted 3 lines

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.

Quoted 6 lines

drivers/video/sunxi_display.c video_hw_init(void) · line 452

Quoted 3 lines

drivers/video/sunxi_display.c +3 -0

+	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.

Quoted 4 lines

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.

Quoted 3 lines

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.

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

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 6 lines · Ian Campbell

Quoted 2 lines

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.

Quoted 16 lines · Hans de Goede

Quoted 11 lines · Ian Campbell

Quoted 9 lines · Hans de Goede

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.

Quoted 3 lines

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.

Quoted 10 lines

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.

Quoted 3 lines

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.

Quoted 1 line

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.

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

Quoted 1 line · Ian Campbell

On Sun, 2014-11-16 at 16:11 +0100, Hans de Goede wrote:

Quoted 11 lines

Quoted 10 lines

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.

Quoted 5 lines

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)

Quoted 10 lines

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 /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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

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.

Quoted 2 lines

[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.

Quoted 3 lines · Grant Likely

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.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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" ;-).

Quoted 12 lines

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.

Quoted 8 lines

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.

Quoted 5 lines

Quoted 2 lines

[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! :-)

:-)

Quoted 4 lines

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.

Quoted 7 lines

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.

For use together with the hdmi console.

Signed-off-by Hans de Goede <[email protected]>

---
 board/sunxi/Kconfig            |  7 +++++++
 configs/Ippo_q8h_v5_defconfig  |  1 +
 include/configs/sunxi-common.h | 25 +++++++++++++++++++++----
 3 files changed, 29 insertions(+), 4 deletions(-)

board/sunxi/Kconfig +0 -0

index 422033a..246cd9a 100644

board/sunxi/Kconfig +6 -0

+	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

configs/Ippo_q8h_v5_defconfig +0 -0

index 53df213..50c2f93 100644

configs/Ippo_q8h_v5_defconfig +9 -0

@@ -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

configs/Ippo_q8h_v5_defconfig +0 -0

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

Quoted 17 lines · Hans de Goede

For use together with the hdmi console.

Signed-off-by Hans de Goede <[email protected]>

---
 board/sunxi/Kconfig            |  7 +++++++
 configs/Ippo_q8h_v5_defconfig  |  1 +
 include/configs/sunxi-common.h | 25 +++++++++++++++++++++----
 3 files changed, 29 insertions(+), 4 deletions(-)

board/sunxi/Kconfig +0 -0

index 422033a..246cd9a 100644
@@ -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.
 

board/sunxi/Kconfig config VIDEO · line 226

Quoted 1 line

board/sunxi/Kconfig +1 -0

+config USB_KEYBOARD

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

Quoted 12 lines

board/sunxi/Kconfig +6 -0

+	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

configs/Ippo_q8h_v5_defconfig +0 -0

index 53df213..50c2f93 100644

configs/Ippo_q8h_v5_defconfig CONFIG_MACH_SUN8I=y · line 8

Quoted 5 lines

configs/Ippo_q8h_v5_defconfig +1 -0

@@ -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?

Quoted 20 lines

configs/Ippo_q8h_v5_defconfig +10 -2

@@ -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

configs/Ippo_q8h_v5_defconfig line 323

Quoted 4 lines

configs/Ippo_q8h_v5_defconfig +3 -1

-#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?

configs/Ippo_q8h_v5_defconfig line 328

Quoted 7 lines

configs/Ippo_q8h_v5_defconfig +2 -1

 #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.

Hi,

Quoted 18 lines · Ian Campbell

Quoted 17 lines · Hans de Goede

For use together with the hdmi console.

Signed-off-by Hans de Goede <[email protected]>

---
 board/sunxi/Kconfig            |  7 +++++++
 configs/Ippo_q8h_v5_defconfig  |  1 +
 include/configs/sunxi-common.h | 25 +++++++++++++++++++++----
 3 files changed, 29 insertions(+), 4 deletions(-)

board/sunxi/Kconfig +0 -0

index 422033a..246cd9a 100644
@@ -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.
 

Quoted 4 lines

board/sunxi/Kconfig config VIDEO · line 226

Quoted 1 line

board/sunxi/Kconfig +1 -0

+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.

Quoted 11 lines

Quoted 11 lines

board/sunxi/Kconfig +6 -0

+	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

configs/Ippo_q8h_v5_defconfig +0 -0

index 53df213..50c2f93 100644

Quoted 8 lines

configs/Ippo_q8h_v5_defconfig CONFIG_MACH_SUN8I=y · line 8

Quoted 5 lines

configs/Ippo_q8h_v5_defconfig +1 -0

@@ -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.

Quoted 20 lines

Quoted 20 lines

configs/Ippo_q8h_v5_defconfig +10 -2

@@ -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

Quoted 8 lines

configs/Ippo_q8h_v5_defconfig line 323

Quoted 4 lines

configs/Ippo_q8h_v5_defconfig +3 -1

-#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.

Quoted 2 lines

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).

Quoted 26 lines · Hans de Goede

Hi,

Quoted 22 lines · Ian Campbell

Quoted 14 lines · Hans de Goede

For use together with the hdmi console.

Signed-off-by Hans de Goede <[email protected]>

---
 board/sunxi/Kconfig            |  7 +++++++
 configs/Ippo_q8h_v5_defconfig  |  1 +
 include/configs/sunxi-common.h | 25 +++++++++++++++++++++----
 3 files changed, 29 insertions(+), 4 deletions(-)

board/sunxi/Kconfig +0 -0

index 422033a..246cd9a 100644

board/sunxi/Kconfig config VIDEO · line 226

Quoted 4 lines

board/sunxi/Kconfig +1 -0

@@ -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.

Quoted 4 lines

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?

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

board/sunxi/Kconfig +6 -0

+	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

configs/Ippo_q8h_v5_defconfig +0 -0

index 53df213..50c2f93 100644

Quoted 8 lines

configs/Ippo_q8h_v5_defconfig CONFIG_MACH_SUN8I=y · line 8

Quoted 5 lines

configs/Ippo_q8h_v5_defconfig +1 -0

@@ -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?

Quoted 2 lines

You mean without video + usb, since the default is y, and it is being forced to n here.

Sorry, I read this backwards somehow!

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 34 lines

Quoted 20 lines

Quoted 20 lines

configs/Ippo_q8h_v5_defconfig +10 -2

@@ -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

Quoted 8 lines

configs/Ippo_q8h_v5_defconfig line 323

Quoted 4 lines

configs/Ippo_q8h_v5_defconfig +3 -1

-#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.

Quoted 3 lines

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.