Re: [U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi
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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi
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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi
On Sun, 2014-11-16 at 11:35 +, 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi
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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi
Hi, On 11/14/2014 09:24 PM, Anatolij Gustschin wrote: > On Fri, 14 Nov 2014 21:17:39 +0100 > Anatolij Gustschin 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 And then add your acked-by. Regards, Hans ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi
On Fri, 14 Nov 2014 21:17:39 +0100 Anatolij Gustschin 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi
Hi Hans,
this patch looks good, only a few minor comments below.
On Fri, 14 Nov 2014 17:54:45 +0100
Hans de Goede 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 000..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(100 / 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi
From: Luc Verhaegen
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
[hdego...@redhat.com: Major cleanups and some small bugfixes]
Signed-off-by: Hans de Goede
---
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 000..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
+ *
+ * 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;
