ARM: vf610: Add DCU framebuffer driver for Vybrid VF610 platform
Subsystems (7)
v2 cover ARM: vf610: Add DCU framebuffer driver for Vybrid VF610 platform
This series contain DCU framebuffer driver for Freescale Vybrid VF610 platform.
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU intervention.
The features:
(1) Full RGB888 output to TFT LCD panel.
(2) For the current LCD panel, WQVGA "480x272" is tested.
(3) Blending of each pixel using up to 4 source layers dependent on size of panel.
(4) Each graphic layer can be placed with one pixel resolution in either axis.
(5) Each graphic layer support RGB565 and RGB888 direct colors without alpha channel and BGRA8888 direct colors with an alpha channel.
(6) Each graphic layer support alpha blending with 8-bit resolution.
Changes in v2:
- Add a document for DCU framebuffer driver under Documentation/devicetree/bindings/fb/.
----------------------------------------------------------------
Alison Wang (5):
ARM: dts: vf610: Add DCU and TCON nodes
ARM: dts: vf610-twr: Enable DCU and TCON devices
ARM: clk: vf610: Add DCU and TCON clock support
fb: Add DCU framebuffer driver for Vybrid VF610 platform
Documentation: DT: Add DCU framebuffer driver
Documentation/devicetree/bindings/fb/fsl-dcu-fb.txt | 36 ++++ arch/arm/boot/dts/vf610-twr.dts | 10 + arch/arm/boot/dts/vf610.dtsi | 19 +- arch/arm/mach-imx/clk-vf610.c | 5 + drivers/video/Kconfig | 9 + drivers/video/Makefile | 1 + drivers/video/fsl-dcu-fb.c | 1091 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ include/dt-bindings/clock/vf610-clock.h | 3 +- 8 files changed, 1172 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/fb/fsl-dcu-fb.txt create mode 100644 drivers/video/fsl-dcu-fb.c
PATCH v2 1/5 ARM: dts: vf610: Add DCU and TCON nodes
This patch adds DCU and TCON nodes in SoC level DTS for Freescale Vybrid VF610. It also removes useless pin for DCU0 pinctrl.
---
Changes in v2: None
arch/arm/boot/dts/vf610.dtsi | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
arch/arm/boot/dts/vf610.dtsi +18 -1
@@ -140,6 +140,14 @@ clock-names = "pit"; }; + tcon0: tcon at 4003d000 { + compatible = "fsl,vf610-tcon"; + reg = <0x4003d000 0x1000>; + clocks = <&clks VF610_CLK_TCON0>; + clock-names = "tcon"; + status = "disabled"; + }; + wdog at 4003e000 { compatible = "fsl,vf610-wdt", "fsl,imx21-wdt"; reg = <0x4003e000 0x1000>;
@@ -169,7 +177,6 @@ dcu0 { pinctrl_dcu0_1: dcu0grp_1 { fsl,pins = < - VF610_PAD_PTB8__GPIO_30 0x42 VF610_PAD_PTE0__DCU0_HSYNC 0x42 VF610_PAD_PTE1__DCU0_VSYNC 0x42 VF610_PAD_PTE2__DCU0_PCLK 0x42
@@ -395,6 +402,16 @@ reg = <0x40050000 0x1000>; }; + dcu0: dcu at 40058000 { + compatible = "fsl,vf610-dcu"; + reg = <0x40058000 0x1200>; + interrupts = <0 30 0x04>; + clocks = <&clks VF610_CLK_DCU0>; + clock-names = "dcu"; + tcon-controller = <&tcon0>; + status = "disabled"; + }; + i2c0: i2c at 40066000 { #address-cells = <1>; #size-cells = <0>;
Signature · 1 line
1.8.0
PATCH v2 2/5 ARM: dts: vf610-twr: Enable DCU and TCON devices
In reply to Alison WangThis patch enables DCU and TCON devices for Vybrid VF610 TOWER board.
---
Changes in v2: None
arch/arm/boot/dts/vf610-twr.dts | 10 ++++++++++ 1 file changed, 10 insertions(+)
arch/arm/boot/dts/vf610-twr.dts +10 -0
@@ -36,6 +36,12 @@ }; +&dcu0 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_dcu0_1>; + status = "okay"; +}; + &fec0 { phy-mode = "rmii"; pinctrl-names = "default";
@@ -50,6 +56,10 @@ status = "okay"; }; +&tcon0 { + status = "okay"; +}; + &uart1 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_uart1_1>;
Signature · 1 line
1.8.0
PATCH v2 3/5 ARM: clk: vf610: Add DCU and TCON clock support
In reply to Alison WangThis patch adds DCU and TCON clock support for Vybrid VF610 platform.
---
Changes in v2: None
arch/arm/mach-imx/clk-vf610.c | 5 +++++ include/dt-bindings/clock/vf610-clock.h | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-)
arch/arm/mach-imx/clk-vf610.c +5 -0
@@ -245,6 +245,8 @@ static void __init vf610_clocks_init(struct device_node *ccm_node) clk[VF610_CLK_DCU1_DIV ] =imx_clk_divider ("dcu1_div" ,"dcu1_en" ,CCM_CSCDR3 ,20 ,3 ); clk[VF610_CLK_DCU1 ] =imx_clk_gate2 ("dcu1" ,"dcu1_div" ,CCM_CCGR9 ,CCM_CCGRx_CGn (8 )); + clk[VF610_CLK_TCON0 ] =imx_clk_gate2 ("tcon0" ,"platform_bus" ,CCM_CCGR1 ,CCM_CCGRx_CGn (13 )); + clk[VF610_CLK_ESAI_SEL ] =imx_clk_mux ("esai_sel" ,CCM_CSCMR1 ,20 ,2 , esai_sels,4 ); clk[VF610_CLK_ESAI_EN ] =imx_clk_gate ("esai_en" ,"esai_sel" ,CCM_CSCDR2 ,30 ); clk[VF610_CLK_ESAI_DIV ] =imx_clk_divider ("esai_div" ,"esai_en" ,CCM_CSCDR2 ,24 ,4 );
@@ -311,6 +313,9 @@ static void __init vf610_clocks_init(struct device_node *ccm_node)clk_set_parent (clk[VF610_CLK_SAI2_SEL ], clk[VF610_CLK_AUDIO_EXT ]);clk_set_parent (clk[VF610_CLK_SAI3_SEL ], clk[VF610_CLK_AUDIO_EXT ]); +clk_set_parent (clk[VF610_CLK_DCU0_SEL ], clk[VF610_CLK_PLL1_PFD2 ]); +clk_set_rate (clk[VF610_CLK_DCU0_DIV ],113200000 ); +/* Add the clocks to provider list */ clk_data.clks = clk; clk_data.clk_num =ARRAY_SIZE (clk);
include/dt-bindings/clock/vf610-clock.h +2 -1
@@ -158,6 +158,7 @@#define VF610_CLK_GPU_SEL 145#define VF610_CLK_GPU_EN 146#define VF610_CLK_GPU2D 147 -#define VF610_CLK_END 148 +#define VF610_CLK_TCON0 148 +#define VF610_CLK_END 149#endif /* __DT_BINDINGS_CLOCK_VF610_H */
Signature · 1 line
1.8.0
PATCH v2 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
In reply to Alison WangThe Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU intervention.
The features:
(1) Full RGB888 output to TFT LCD panel.
(2) For the current LCD panel, WQVGA "480x272" is tested.
(3) Blending of each pixel using up to 4 source layers dependent on size of panel.
(4) Each graphic layer can be placed with one pixel resolution in either axis.
(5) Each graphic layer support RGB565 and RGB888 direct colors without alpha channel and BGRA8888 direct colors with an alpha channel.
(6) Each graphic layer support alpha blending with 8-bit resolution.
This driver has been tested on Vybrid VF610 TOWER board.
---
Changes in v2: None
drivers/video/Kconfig | 9 + drivers/video/Makefile | 1 + drivers/video/fsl-dcu-fb.c | 1091 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1101 insertions(+) create mode 100644 drivers/video/fsl-dcu-fb.c
drivers/video/Kconfig +9 -0
@@ -1978,6 +1978,15 @@ config FB_FSL_DIU ---help--- Framebuffer driver for the Freescale SoC DIU +config FB_FSL_DCU + tristate "Freescale DCU framebuffer support" + depends on FB + select FB_CFB_FILLRECT + select FB_CFB_COPYAREA + select FB_CFB_IMAGEBLIT + ---help--- + Framebuffer driver for the Freescale SoC DCU + config FB_W100 tristate "W100 frame buffer support" depends on FB && ARCH_PXA
drivers/video/Makefile +1 -0
@@ -129,6 +129,7 @@ obj-$(CONFIG_FB_IMX) += imxfb.o obj-$(CONFIG_FB_S3C) += s3c-fb.o obj-$(CONFIG_FB_S3C2410) += s3c2410fb.o obj-$(CONFIG_FB_FSL_DIU) += fsl-diu-fb.o +obj-$(CONFIG_FB_FSL_DCU) += fsl-dcu-fb.o obj-$(CONFIG_FB_COBALT) += cobalt_lcdfb.o obj-$(CONFIG_FB_IBM_GXT4500) += gxt4500.o obj-$(CONFIG_FB_PS3) += ps3fb.o
drivers/video/fsl-dcu-fb.c new file +1091 -0
1,091 lines not on this page · raw message
Signature · 1 line
1.8.0
Sascha Hauer
maintainerPATCH 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
Quoted 6 lines · Alison Wang
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU intervention.
Only a review of the code inline.
Maybe the real question is whether we want to introduce another framebuffer driver at all instead of making it a DRM driver.
drivers/video/fsl-dcu-fb.c line 28
Quoted 5 lines
drivers/video/fsl-dcu-fb.c new file +5
-0+ +#define DRIVER_NAME "fsl-dcu-fb" + +#define DCU_DCU_MODE 0x0010 +#define DCU_MODE_BLEND_ITER (x) (x << 20)
What's the result of DCU_MODE_BLEND_ITER(1 + 1)?
Quoted 7 lines
drivers/video/fsl-dcu-fb.c new file +7
-0+static struct fb_videomode dcu_mode_db[] = { + { + .name ="480x272" , + .refresh =75 , + .xres =480 , + .yres =272 , + .pixclock =91996 ,
drivers/video/fsl-dcu-fb.c line 156
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+ .left_margin = 2, + .right_margin = 2, + .upper_margin = 1, + .lower_margin = 1, + .hsync_len = 41, + .vsync_len = 2, + .sync = FB_SYNC_COMP_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT, + .vmode = FB_VMODE_NONINTERLACED, + }, +};
We have ways to describe a panel in dt. Use them.
drivers/video/fsl-dcu-fb.c line 199
Quoted 11 lines
drivers/video/fsl-dcu-fb.c new file +11
-0+ +static struct mfb_info mfb_template[] = { + { + .index =LAYER0 , + .id ="Layer0" , + .alpha =0xff , + .blend =0 , + .count =0 , + .x_layer_d =0 , + .y_layer_d =0 , + },
Wrong indentation.
drivers/video/fsl-dcu-fb.c line 258
Quoted 3 lines
drivers/video/fsl-dcu-fb.c new file +3
-0+default : +printk (KERN_ERR "unsupported color depth: %u\n" , + var->bits_per_pixel);
Use dev_* for printing messages in drivers.
drivers/video/fsl-dcu-fb.c line 356
Quoted 13 lines
drivers/video/fsl-dcu-fb.c new file +13
-0+static int fsl_dcu_check_var (struct fb_var_screeninfo *var, +struct fb_info *info) +{ +if (var->xres_virtual < var->xres) + var->xres_virtual = var->xres; +if (var->yres_virtual < var->yres) + var->yres_virtual = var->yres; + +if (var->xoffset <0 ) + var->xoffset =0 ; + +if (var->yoffset <0 ) + var->yoffset =0 ;
Ever seen an unsigned value going below zero?
drivers/video/fsl-dcu-fb.c line 258
Quoted 3 lines
drivers/video/fsl-dcu-fb.c new file +3
-0+default : +printk (KERN_ERR "unsupported color depth: %u\n" , + var->bits_per_pixel);
BUG(). This can't happen since you make that sure above.
drivers/video/fsl-dcu-fb.c line 430
Quoted 8 lines
drivers/video/fsl-dcu-fb.c new file +8
-0+return -EINVAL ; + } + +return 0 ; +} + +static int calc_div_ratio (struct fb_info *info) +{
Use a consistent namespace for function names (fsl_dcu_)
drivers/video/fsl-dcu-fb.c line 487
Quoted 5 lines
drivers/video/fsl-dcu-fb.c new file +5
-0+writel (DCU_THRESHOLD_LS_BF_VS (0x3 ) |DCU_THRESHOLD_OUT_BUF_HIGH (0x78 ) | +DCU_THRESHOLD_OUT_BUF_LOW (0 ), dcufb->reg_base +DCU_THRESHOLD ); + +enable_controller (info); +}
Make your functions symmetric. If there's update_controller(), the function should do exactly that, it should *not* enable the controller. Call this from the users of this function if necessary.
drivers/video/fsl-dcu-fb.c line 697
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+if (copy_to_user (buf, &alpha,sizeof (alpha))) +return -EFAULT ; +break ; +case MFB_SET_ALPHA : +if (copy_from_user (&alpha, buf,sizeof (alpha))) +return -EFAULT ; + mfbi->blend =1 ; + mfbi->alpha = alpha; +fsl_dcu_set_par (info); +break ;
Is it still state of the art to introduce ioctls in the fb framework without any kind of documentation?
drivers/video/fsl-dcu-fb.c line 699
Quoted 2 lines
drivers/video/fsl-dcu-fb.c new file +2
-0+default : +printk (KERN_ERR "unknown ioctl command (0x%08X)\n" , cmd);
What shall a reader of the kernel log do with a message like this? It's utterly useless when he doesn't even now which device failed here. Just drop this.
drivers/video/fsl-dcu-fb.c line 711
Quoted 6 lines
drivers/video/fsl-dcu-fb.c new file +6
-0+static void enable_interrupts (struct dcu_fb_data *dcufb) +{ +u32 int_mask =readl (dcufb->reg_base +DCU_INT_MASK ); + +writel (int_mask & ~DCU_INT_MASK_UNDRUN , dcufb->reg_base +DCU_INT_MASK ); +}
Inline this code where you need it. Introducing a function for a single register write seems quite useless.
Quoted 9 lines
drivers/video/fsl-dcu-fb.c new file +9
-0+static int install_framebuffer (struct fb_info *info) +{ +struct mfb_info *mfbi = info->par; +struct fb_videomode *db = dcu_mode_db; +unsigned int dbsize =ARRAY_SIZE (dcu_mode_db); +int ret; + + info->var.activate =FB_ACTIVATE_NOW ; + info->fbops = &fsl_dcu_ops;
drivers/video/fsl-dcu-fb.c line 799
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+ info->flags =FBINFO_FLAG_DEFAULT ; + info->pseudo_palette = &mfbi->pseudo_palette; + +fb_alloc_cmap (&info->cmap,16 ,0 ); + + ret =fb_find_mode (&info->var, info, fb_mode, db, dbsize, +NULL , default_bpp); + +if (fsl_dcu_check_var (&info->var, info)) { + ret = -EINVAL ;
Propagate the error.
drivers/video/fsl-dcu-fb.c line 804
Quoted 5 lines
drivers/video/fsl-dcu-fb.c new file +5
-0+ goto failed_checkvar; + } + +if (register_framebuffer (info) <0 ) { + ret = -EINVAL ;
ditto
drivers/video/fsl-dcu-fb.c line 835
Quoted 7 lines
drivers/video/fsl-dcu-fb.c new file +7
-0+static irqreturn_t fsl_dcu_irq (int irq,void *dev_id) +{ +struct dcu_fb_data *dcufb = dev_id; +unsigned int status =readl (dcufb->reg_base +DCU_INT_STATUS ); +u32 dcu_mode; + +if (status) {
Save indendation level by bailing out early.
Quoted 12 lines
drivers/video/fsl-dcu-fb.c new file +12
-0+if (status &DCU_INT_STATUS_UNDRUN ) { + dcu_mode =readl (dcufb->reg_base +DCU_DCU_MODE ); + dcu_mode &= ~DCU_MODE_DCU_MODE_MASK ; +writel (dcu_mode |DCU_MODE_DCU_MODE (DCU_MODE_OFF ), + dcufb->reg_base +DCU_DCU_MODE ); +udelay (1 ); +writel (dcu_mode |DCU_MODE_DCU_MODE (DCU_MODE_NORMAL ), + dcufb->reg_base +DCU_DCU_MODE ); + } +writel (status, dcufb->reg_base +DCU_INT_STATUS ); +return IRQ_HANDLED ; + }
drivers/video/fsl-dcu-fb.c line 896
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+return IRQ_NONE ; +} + +if (IS_ERR (tcon_clk)) { + ret =PTR_ERR (tcon_clk); + goto failed_getclock; + } +clk_prepare_enable (tcon_clk); + + tcon_reg =of_iomap (tcon_np,0 );
Use devm_*
drivers/video/fsl-dcu-fb.c line 943
Quoted 7 lines
drivers/video/fsl-dcu-fb.c new file +7
-0+ dcufb->irq =platform_get_irq (pdev,0 ); +if (!dcufb->irq) { + ret = -EINVAL ; + goto failed_getirq; + } + + ret =request_irq (dcufb->irq, fsl_dcu_irq,0 ,DRIVER_NAME , dcufb);
Use devm_request_irq
Quoted 7 lines
drivers/video/fsl-dcu-fb.c new file +7
-0+if (ret) { +dev_err (&pdev->dev,"could not request irq\n" ); + goto failed_requestirq; + } + +/* Put TCON in bypass mode, so the input signals from DCU are passed +* through TCON unchanged */
drivers/video/fsl-dcu-fb.c line 960
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+ ret =bypass_tcon (pdev->dev.of_node); +if (ret) { +dev_err (&pdev->dev,"could not bypass TCON\n" ); + goto failed_bypasstcon; + } + + dcufb->clk =devm_clk_get (&pdev->dev,"dcu" ); +if (IS_ERR (dcufb->clk)) { +dev_err (&pdev->dev,"could not get clock\n" ); + goto failed_getclock;
You will return 0 here.
drivers/video/fsl-dcu-fb.c line 968
Quoted 8 lines
drivers/video/fsl-dcu-fb.c new file +8
-0+ } +clk_prepare_enable (dcufb->clk); + +for (i =0 ; i <ARRAY_SIZE (dcufb->fsl_dcu_info); i++) { + dcufb->fsl_dcu_info[i] = +framebuffer_alloc (sizeof (struct mfb_info ), &pdev->dev); +if (!dcufb->fsl_dcu_info[i]) { + ret =ENOMEM ;
-ENOMEM
drivers/video/fsl-dcu-fb.c line 1002
Quoted 7 lines
drivers/video/fsl-dcu-fb.c new file +7
-0+failed_alloc_framebuffer: +failed_getclock: +failed_bypasstcon: +free_irq (dcufb->irq, dcufb); +failed_requestirq: +failed_getirq: +iounmap (dcufb->reg_base);
You used devm_ioremap, so drop this.
drivers/video/fsl-dcu-fb.c line 1004
Quoted 2 lines
drivers/video/fsl-dcu-fb.c new file +2
-0+failed_ioremap: +kfree (dcufb);
This is allocated with devm_*. Drop this.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH v2 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
Hi, Sascha,
Quoted 7 lines · Alison Wang
Quoted 6 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU
Quoted 6 lines
intervention.
Only a review of the code inline.
Maybe the real question is whether we want to introduce another framebuffer driver at all instead of making it a DRM driver.
[Alison Wang] I think DCU module is more suitable to be designed as a framebuffer driver than a DRM driver. Just like DIU framebuffer driver for PowerPC.
Quoted 8 lines
drivers/video/fsl-dcu-fb.c line 28
Quoted 6 lines
drivers/video/fsl-dcu-fb.c new file +5
-0+ +#define DRIVER_NAME "fsl-dcu-fb" + +#define DCU_DCU_MODE 0x0010 +#define DCU_MODE_BLEND_ITER (x) (x << 20)What's the result of
DCU_MODE_BLEND_ITER(1 + 1)?
[Alison Wang] Is it really necessary? I don't use this macro like DCU_MODE_BLEND_ITER(1 + 1), just use DCU_MODE_BLEND_ITER(2).
Quoted 12 lines
Quoted 12 lines
drivers/video/fsl-dcu-fb.c new file +11
-0+static struct fb_videomode dcu_mode_db[] = { + { + .name ="480x272" , + .refresh =75 , + .xres =480 , + .yres =272 , + .pixclock =91996 , + .left_margin =2 , + .right_margin =2 , + .upper_margin =1 , + .lower_margin =1 ,
Quoted 8 lines
drivers/video/fsl-dcu-fb.c line 156
Quoted 6 lines
drivers/video/fsl-dcu-fb.c new file +6
-0+ .hsync_len = 41, + .vsync_len = 2, + .sync = FB_SYNC_COMP_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT, + .vmode = FB_VMODE_NONINTERLACED, + }, +};We have ways to describe a panel in dt. Use them.
[Alison Wang] Ok. I don't know it is common to describe the panel in dts for the latest kernel. Thanks.
Quoted 14 lines
drivers/video/fsl-dcu-fb.c line 199
Quoted 12 lines
drivers/video/fsl-dcu-fb.c new file +11
-0+ +static struct mfb_info mfb_template[] = { + { + .index =LAYER0 , + .id ="Layer0" , + .alpha =0xff , + .blend =0 , + .count =0 , + .x_layer_d =0 , + .y_layer_d =0 , + },Wrong indentation.
[Alison Wang] I will fix it in next version.
Quoted 6 lines
drivers/video/fsl-dcu-fb.c line 258
Quoted 4 lines
drivers/video/fsl-dcu-fb.c new file +3
-0+default : +printk (KERN_ERR "unsupported color depth: %u\n" , + var->bits_per_pixel);Use dev_* for printing messages in drivers.
[Alison Wang] Ok.
Quoted 16 lines
Quoted 4 lines
drivers/video/fsl-dcu-fb.c new file +3
-0+static int fsl_dcu_check_var (struct fb_var_screeninfo *var, +struct fb_info *info) +{drivers/video/fsl-dcu-fb.c line 356
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+if (var->xres_virtual < var->xres) + var->xres_virtual = var->xres; +if (var->yres_virtual < var->yres) + var->yres_virtual = var->yres; + +if (var->xoffset <0 ) + var->xoffset =0 ; + +if (var->yoffset <0 ) + var->yoffset =0 ;Ever seen an unsigned value going below zero?
[Alison Wang] You are right, I will remove them in next version.
Quoted 6 lines
drivers/video/fsl-dcu-fb.c line 258
Quoted 4 lines
drivers/video/fsl-dcu-fb.c new file +3
-0+default : +printk (KERN_ERR "unsupported color depth: %u\n" , + var->bits_per_pixel);
BUG(). This can't happen since you make that sure above.
[Alison Wang] You are right, I will remove it in next version.
Quoted 10 lines
drivers/video/fsl-dcu-fb.c
Quoted 8 lines
drivers/video/fsl-dcu-fb.c new file +7
-0+return -EINVAL ; + } + +return 0 ; +} + +static int calc_div_ratio (struct fb_info *info) {Use a consistent namespace for function names (
fsl_dcu_)
[Alison Wang] Is it necessary to use a consistent namespace for this generic function? I think it is necessary to use a consistent namespace (fsl_dcu_) for the function names in structure fsl_dcu_ops.
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +1
-0+ writel(DCU_THRESHOLD_LS_BF_VS(0x3) |
DCU_THRESHOLD_OUT_BUF_HIGH(0x78)|Review
Quoted 1 line
(quoted diff) +1
-0+ DCU_THRESHOLD_OUT_BUF_LOW(0), dcufb->reg_base +
DCU_THRESHOLD);
Quoted 7 lines
drivers/video/fsl-dcu-fb.c line 487
Quoted 3 lines
drivers/video/fsl-dcu-fb.c new file +3
-0+ +enable_controller (info); +}Make your functions symmetric. If there's
update_controller(), the function should do exactly that, it should *not* enable the controller. Call this from the users of this function if necessary.
[Alison Wang] Ok, I will move this function out, and add it here.
if (mfbi->index == LAYER0) {
update_controller(info);+ enable_controller(info);
}
Quoted 14 lines
drivers/video/fsl-dcu-fb.c line 697
Quoted 11 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+if (copy_to_user (buf, &alpha,sizeof (alpha))) +return -EFAULT ; +break ; +case MFB_SET_ALPHA : +if (copy_from_user (&alpha, buf,sizeof (alpha))) +return -EFAULT ; + mfbi->blend =1 ; + mfbi->alpha = alpha; +fsl_dcu_set_par (info); +break ;Is it still state of the art to introduce ioctls in the fb framework without any kind of documentation?
[Alison Wang] Do you mean I need to add a documentation in Documentation/fb/?
Quoted 7 lines
drivers/video/fsl-dcu-fb.c line 699
Quoted 3 lines
drivers/video/fsl-dcu-fb.c new file +2
-0+default : +printk (KERN_ERR "unknown ioctl command (0x%08X)\n" , cmd);What shall a reader of the kernel log do with a message like this? It's utterly useless when he doesn't even now which device failed here. Just drop this.
[Alison Wang] Ok.
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +5
-0+static void enable_interrupts(struct dcu_fb_data *dcufb) { + u32 int_mask = readl(dcufb->reg_base + DCU_INT_MASK); + + writel(int_mask & ~DCU_INT_MASK_UNDRUN, dcufb->reg_base + +DCU_INT_MASK); }Inline this code where you need it. Introducing a function for a single register write seems quite useless.
[Alison Wang] Ok, I will inline it.
Quoted 13 lines
Quoted 13 lines
drivers/video/fsl-dcu-fb.c new file +12
-0+static int install_framebuffer (struct fb_info *info) { +struct mfb_info *mfbi = info->par; +struct fb_videomode *db = dcu_mode_db; +unsigned int dbsize =ARRAY_SIZE (dcu_mode_db); +int ret; + + info->var.activate =FB_ACTIVATE_NOW ; + info->fbops = &fsl_dcu_ops; + info->flags =FBINFO_FLAG_DEFAULT ; + info->pseudo_palette = &mfbi->pseudo_palette; + +fb_alloc_cmap (&info->cmap,16 ,0 );
Quoted 8 lines
drivers/video/fsl-dcu-fb.c line 799
Quoted 6 lines
drivers/video/fsl-dcu-fb.c new file +6
-0+ + ret =fb_find_mode (&info->var, info, fb_mode, db, dbsize, +NULL , default_bpp); + +if (fsl_dcu_check_var (&info->var, info)) { + ret = -EINVAL ;Propagate the error.
[Alison Wang] I will fix in next version.
Quoted 8 lines
drivers/video/fsl-dcu-fb.c line 804
Quoted 6 lines
drivers/video/fsl-dcu-fb.c new file +5
-0+ goto failed_checkvar; + } + +if (register_framebuffer (info) <0 ) { + ret = -EINVAL ;ditto
[Alison Wang] I will fix in next version.
Quoted 9 lines
drivers/video/fsl-dcu-fb.c line 835
Quoted 7 lines
drivers/video/fsl-dcu-fb.c new file +6
-0+static irqreturn_t fsl_dcu_irq (int irq,void *dev_id) { +struct dcu_fb_data *dcufb = dev_id; +unsigned int status =readl (dcufb->reg_base +DCU_INT_STATUS ); +u32 dcu_mode; + +if (status) {Save indendation level by bailing out early.
[Alison Wang] What do you mean?
Quoted 17 lines
Quoted 17 lines
drivers/video/fsl-dcu-fb.c new file +16
-0+if (status &DCU_INT_STATUS_UNDRUN ) { + dcu_mode =readl (dcufb->reg_base +DCU_DCU_MODE ); + dcu_mode &= ~DCU_MODE_DCU_MODE_MASK ; +writel (dcu_mode |DCU_MODE_DCU_MODE (DCU_MODE_OFF ), + dcufb->reg_base +DCU_DCU_MODE ); +udelay (1 ); +writel (dcu_mode |DCU_MODE_DCU_MODE (DCU_MODE_NORMAL ), + dcufb->reg_base +DCU_DCU_MODE ); + } +writel (status, dcufb->reg_base +DCU_INT_STATUS ); +return IRQ_HANDLED ; + } +return IRQ_NONE ; +} + +if (IS_ERR (tcon_clk)) {
Quoted 8 lines
drivers/video/fsl-dcu-fb.c line 896
Quoted 6 lines
drivers/video/fsl-dcu-fb.c new file +6
-0+ ret =PTR_ERR (tcon_clk); + goto failed_getclock; + } +clk_prepare_enable (tcon_clk); + + tcon_reg =of_iomap (tcon_np,0 );Use devm_*
[Alison Wang] Ok.
Quoted 10 lines
drivers/video/fsl-dcu-fb.c line 943
Quoted 8 lines
drivers/video/fsl-dcu-fb.c new file +7
-0+ dcufb->irq =platform_get_irq (pdev,0 ); +if (!dcufb->irq) { + ret = -EINVAL ; + goto failed_getirq; + } + + ret =request_irq (dcufb->irq, fsl_dcu_irq,0 ,DRIVER_NAME , dcufb);Use
devm_request_irq
[Alison Wang] Ok.
Quoted 8 lines
drivers/video/fsl-dcu-fb.c
Quoted 7 lines
drivers/video/fsl-dcu-fb.c new file +6
-0+if (ret) { +dev_err (&pdev->dev,"could not request irq\n" ); + goto failed_requestirq; + } + +/* Put TCON in bypass mode, so the input signals from DCU are passed
Quoted 13 lines
drivers/video/fsl-dcu-fb.c line 960
Quoted 11 lines
drivers/video/fsl-dcu-fb.c new file +11
-0+* through TCON unchanged */ + ret =bypass_tcon (pdev->dev.of_node); +if (ret) { +dev_err (&pdev->dev,"could not bypass TCON\n" ); + goto failed_bypasstcon; + } + + dcufb->clk =devm_clk_get (&pdev->dev,"dcu" ); +if (IS_ERR (dcufb->clk)) { +dev_err (&pdev->dev,"could not get clock\n" ); + goto failed_getclock;You will return 0 here.
[Alison Wang] You are right, I will fix it in next version.
Quoted 13 lines
drivers/video/fsl-dcu-fb.c line 968
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +6
-0+ } +clk_prepare_enable (dcufb->clk); + +for (i =0 ; i <ARRAY_SIZE (dcufb->fsl_dcu_info); i++) { + dcufb->fsl_dcu_info[i] = + framebuffer_alloc(sizeof(struct mfb_info), &pdev-dev);
drivers/video/fsl-dcu-fb.c new file +2
-0+if (!dcufb->fsl_dcu_info[i]) { + ret =ENOMEM ;
-ENOMEM
Quoted 9 lines
drivers/video/fsl-dcu-fb.c line 1002
Quoted 7 lines
drivers/video/fsl-dcu-fb.c new file +7
-0+failed_alloc_framebuffer: +failed_getclock: +failed_bypasstcon: +free_irq (dcufb->irq, dcufb); +failed_requestirq: +failed_getirq: +iounmap (dcufb->reg_base);You used
devm_ioremap, so drop this.
[Alison Wang] Ok.
Quoted 5 lines
drivers/video/fsl-dcu-fb.c line 1004
Quoted 3 lines
drivers/video/fsl-dcu-fb.c new file +2
-0+failed_ioremap: +kfree (dcufb);This is allocated with devm_*. Drop this.
[Alison Wang] Ok.
Thanks for your comments.
Best Regards,
Alison Wang
Sascha Hauer
maintainerPATCH 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
Quoted 17 lines · On Mon, Aug 05, 2013 at 09:51:40AM +0000, Wang Huan-B18965 wrote:
Hi, Sascha,
Quoted 7 lines · Alison Wang
Quoted 6 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU
Quoted 6 lines
intervention.
Only a review of the code inline.
Maybe the real question is whether we want to introduce another framebuffer driver at all instead of making it a DRM driver.
[Alison Wang] I think DCU module is more suitable to be designed as a framebuffer driver than a DRM driver. Just like DIU framebuffer driver for PowerPC.
Quoted 1 line
Quoted 8 lines
Quoted 7 lines
drivers/video/fsl-dcu-fb.c line 28
Quoted 5 lines
drivers/video/fsl-dcu-fb.c new file +5
-0+ +#define DRIVER_NAME "fsl-dcu-fb" + +#define DCU_DCU_MODE 0x0010 +#define DCU_MODE_BLEND_ITER (x) (x << 20)What's the result of
DCU_MODE_BLEND_ITER(1 + 1)?[Alison Wang] Is it really necessary? I don't use this macro like
DCU_MODE_BLEND_ITER(1 + 1), just useDCU_MODE_BLEND_ITER(2).
<irony>
No it's not necessary. We can just add incorrect macros everywhere and fix them as necessary after a long bug squashing session </irony>
YES! This is necessary.
Quoted 7 lines
Quoted 6 lines
drivers/video/fsl-dcu-fb.c
Quoted 4 lines
drivers/video/fsl-dcu-fb.c new file +4
-0+return 0 ; +} + +static int calc_div_ratio (struct fb_info *info) {Use a consistent namespace for function names (
fsl_dcu_)[Alison Wang] Is it necessary to use a consistent namespace for this generic function? I think it is necessary to use a consistent namespace (
fsl_dcu_) for the function names in structurefsl_dcu_ops.
This function calculates some divider out of a struct fb_info. This is by no means generic.
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
drivers/video/fsl-dcu-fb.c new file +5
-0+if (copy_to_user (buf, &alpha,sizeof (alpha))) +return -EFAULT ; +break ; +case MFB_SET_ALPHA : +if (copy_from_user (&alpha, buf,sizeof (alpha)))Quoted 8 lines
drivers/video/fsl-dcu-fb.c line 697
Quoted 5 lines
drivers/video/fsl-dcu-fb.c new file +5
-0+return -EFAULT ; + mfbi->blend =1 ; + mfbi->alpha = alpha; +fsl_dcu_set_par (info); +break ;Is it still state of the art to introduce ioctls in the fb framework without any kind of documentation?
Quoted 1 line
[Alison Wang] Do you mean I need to add a documentation in
Documentation/fb/?
This was more a question to the fb maintainers.
Quoted 9 lines
Quoted 8 lines
drivers/video/fsl-dcu-fb.c line 835
Quoted 6 lines
drivers/video/fsl-dcu-fb.c new file +6
-0+static irqreturn_t fsl_dcu_irq (int irq,void *dev_id) { +struct dcu_fb_data *dcufb = dev_id; +unsigned int status =readl (dcufb->reg_base +DCU_INT_STATUS ); +u32 dcu_mode; + +if (status) {Save indendation level by bailing out early.
[Alison Wang] What do you mean?
Instead of doing:
if (bla) {
dothis();
dothat();
return;
}
return;it's easier to read:
if (!bla) return; dothis(); dothat(); return;
Note that dothis() and dothat() are one indentation level to the left and thus have more space per line.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH v2 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
Quoted 29 lines · On Mon, Aug 05, 2013 at 09:51:40AM +0000, Wang Huan-B18965 wrote:
Quoted 7 lines
Hi, Sascha,
Quoted 5 lines · Alison Wang
Quoted 4 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly
configurable.
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU
intervention.
Only a review of the code inline.
Maybe the real question is whether we want to introduce another framebuffer driver at all instead of making it a DRM driver.
[Alison Wang] I think DCU module is more suitable to be designed as a
framebuffer driver than a DRM driver. Just like DIU framebuffer driver for PowerPC.
Quoted 8 lines
Quoted 8 lines
drivers/video/fsl-dcu-fb.c line 28
Quoted 6 lines
drivers/video/fsl-dcu-fb.c new file +5
-0+ +#define DRIVER_NAME "fsl-dcu-fb" + +#define DCU_DCU_MODE 0x0010 +#define DCU_MODE_BLEND_ITER (x) (x << 20)What's the result of
DCU_MODE_BLEND_ITER(1 + 1)?
Quoted 8 lines
Quoted 1 line
[Alison Wang] Is it really necessary? I don't use this macro like
DCU_MODE_BLEND_ITER(1 + 1), just useDCU_MODE_BLEND_ITER(2).<irony>
No it's not necessary. We can just add incorrect macros everywhere and fix them as necessary after a long bug squashing session </irony>YES! This is necessary.
[Alison Wang] Ok, I will change it as follows.
#define DCU_MODE_BLEND_ITER(x) ((x) << 20)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/video/fsl-dcu-fb.c
Quoted 5 lines
drivers/video/fsl-dcu-fb.c new file +4
-0+return 0 ; +} + +static int calc_div_ratio (struct fb_info *info) {Use a consistent namespace for function names (
fsl_dcu_)
Quoted 6 lines
Quoted 1 line
[Alison Wang] Is it necessary to use a consistent namespace for this
generic function? I think it is necessary to use a consistent namespace (
fsl_dcu_) for the function names in structurefsl_dcu_ops.This function calculates some divider out of a struct
fb_info. This is by no means generic.
[Alison Wang] Ok, I will use a consistent namespace for this function.
Quoted 51 lines
Quoted 13 lines
Quoted 13 lines
drivers/video/fsl-dcu-fb.c line 697
Quoted 11 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+if (copy_to_user (buf, &alpha,sizeof (alpha))) +return -EFAULT ; +break ; +case MFB_SET_ALPHA : +if (copy_from_user (&alpha, buf,sizeof (alpha))) +return -EFAULT ; + mfbi->blend =1 ; + mfbi->alpha = alpha; +fsl_dcu_set_par (info); +break ;Is it still state of the art to introduce ioctls in the fb
framework
Quoted 2 lines
Quoted 1 line
without any kind of documentation?
[Alison Wang] Do you mean I need to add a documentation in
Documentation/fb/?This was more a question to the fb maintainers.
Quoted 3 lines
Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+static irqreturn_t fsl_dcu_irq(int irq, void *dev_id) { + struct dcu_fb_data *dcufb = dev_id; + unsigned int status = readl(dcufb->reg_base +
DCU_INT_STATUS);Quoted 6 lines
Quoted 5 lines
drivers/video/fsl-dcu-fb.c line 835
Quoted 3 lines
drivers/video/fsl-dcu-fb.c new file +3
-0+u32 dcu_mode; + +if (status) {Save indendation level by bailing out early.
[Alison Wang] What do you mean?
Instead of doing:
if (bla) { dothis(); dothat(); return; } return;it's easier to read:
if (!bla) return; dothis(); dothat(); return;
Quoted 2 lines
Note that
dothis()anddothat()are one indentation level to the left and thus have more space per line.
[Alison Wang] I see. Thanks.
Best Regards,
Alison Wang
PATCH 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
In reply to Wang Huan-B18965Quoted 7 lines · On Mon, Aug 05, 2013 at 09:51:40AM +0000, Wang Huan-B18965 wrote:
Quoted 7 lines · Alison Wang
Quoted 6 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU
Quoted 7 lines
Quoted 6 lines
intervention.
Only a review of the code inline.
Maybe the real question is whether we want to introduce another framebuffer driver at all instead of making it a DRM driver.
[Alison Wang] I think DCU module is more suitable to be designed as a framebuffer driver than a DRM driver. Just like DIU framebuffer driver for PowerPC.
We looked at the Vybrid datasheet and it's DCU section last week, and with its 64 planes, the controller really wants to get a DRM driver.
rsc
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
Am Montag, den 05.08.2013, 15:09 +0200 schrieb Robert Schwebel:
Quoted 10 lines · On Mon, Aug 05, 2013 at 09:51:40AM +0000, Wang Huan-B18965 wrote:
Quoted 9 lines
Quoted 9 lines · Alison Wang
Quoted 6 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU
intervention.
Quoted 9 lines
Quoted 5 lines
Quoted 4 lines
Only a review of the code inline.
Maybe the real question is whether we want to introduce another framebuffer driver at all instead of making it a DRM driver.
[Alison Wang] I think DCU module is more suitable to be designed as a framebuffer driver than a DRM driver. Just like DIU framebuffer driver for PowerPC.
We looked at the Vybrid datasheet and it's DCU section last week, and with its 64 planes, the controller really wants to get a DRM driver.
Exactly, with it's extensive hardware composition capabilities the controller begs for being driven by a proper DRM driver. There is no way in fbdev to properly support all those hardware planes without introducing a lot of non standard ioctls, which in turn will force you into writing a lot of custom userspace code instead of running proven technology that sits on top of KMS.
Doing things in DRM might be slightly more work for the initial bring up, but will pay off in the long run when you are going to really use the hardware caps of the controller.
Regards,
Lucas
Signature · 4 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH v2 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
Hi,
Quoted 33 lines
Am Montag, den 05.08.2013, 15:09 +0200 schrieb Robert Schwebel:
Quoted 3 lines · On Mon, Aug 05, 2013 at 09:51:40AM +0000, Wang Huan-B18965 wrote:
Quoted 2 lines
Quoted 2 lines · Alison Wang
Quoted 1 line
The Display Controller Unit (DCU) module is a system master
that
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes
is
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
supported and the timing of the interface signals is highly
configurable.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU
intervention.
Only a review of the code inline.
Maybe the real question is whether we want to introduce another framebuffer driver at all instead of making it a DRM driver.
[Alison Wang] I think DCU module is more suitable to be designed as
a framebuffer driver than a DRM driver. Just like DIU framebuffer driver for PowerPC.
Quoted 4 lines
We looked at the Vybrid datasheet and it's DCU section last week, and with its 64 planes, the controller really wants to get a DRM driver.
Exactly, with it's extensive hardware composition capabilities the controller begs for being driven by a proper DRM driver. There is no way in fbdev to properly support all those hardware planes without introducing a lot of non standard ioctls, which in turn will force you into writing a lot of custom userspace code instead of running proven technology that sits on top of KMS.
Quoted 4 lines
Doing things in DRM might be slightly more work for the initial bring up, but will pay off in the long run when you are going to really use the hardware caps of the controller.
[Alison Wang] Thanks for your comments and explanations. I understand your considerations.
In DCU module, there are 64 graphics layers, but the maximum source layers for blending of each pixel is only 6. The fb driver could satisfy the current application requirements.
So I will push out this fb driver first, and then I will add a DRM driver for more advanced requirements.
Do you have more comments?
Best Regards,
Alison Wang
PATCH 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
In reply to Sascha HauerQuoted 8 lines · Alison Wang
Quoted 6 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU intervention.
Quoted 2 lines · s.hauer at pengutronix.de
Maybe the real question is whether we want to introduce another framebuffer driver at all instead of making it a DRM driver.
I didn't understand this comment at first. I thought that the DRM infra-structure had changed or something. I see a recent post on IMX-DRM and I think there is a mis-conception on the Vybrid SOC.
At first, Freescale was to incorporate a Vivante GC355 GPU for OpenVG. However, this was removed from the design and is only present on some 'Automotive' parts, and not the VF610 nor the Tower boards. These SOCs only have a multi-level framebuffer with alpha blending. Was it meant that this be part of the DRM? It seems that the hardware without the 'Vivante GC355 GPU' is best served by an fb driver. Certainly, there are very few Vybrid chips with this OpenVG on board. I am also not really certain what sort of user space code would use it. Most graphics stacks seems to want OpenGL; of course that is not a reason not to put it in the kernel, but I don't think any Linux Vybrid devices will actually have an OpenVG register set on board? The majority will have none.
Fwiw,
Bill Pringlemeir.
PATCH 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
Am Montag, den 06.01.2014, 13:50 -0500 schrieb Bill Pringlemeir:
Quoted 17 lines
Quoted 7 lines · Alison Wang
Quoted 6 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU intervention.
Quoted 2 lines · s.hauer at pengutronix.de
Maybe the real question is whether we want to introduce another framebuffer driver at all instead of making it a DRM driver.
I didn't understand this comment at first. I thought that the DRM infra-structure had changed or something. I see a recent post on IMX-DRM and I think there is a mis-conception on the Vybrid SOC.
Quoted 6 lines
At first, Freescale was to incorporate a Vivante GC355 GPU for OpenVG. However, this was removed from the design and is only present on some 'Automotive' parts, and not the VF610 nor the Tower boards. These SOCs only have a multi-level framebuffer with alpha blending. Was it meant that this be part of the DRM? It seems that the hardware without the 'Vivante GC355 GPU' is best served by an fb driver.
Exactly the multi-level part of the hardware should preferably be programmed through the standardized plane stuff in the KMS userspace interface, which is part of DRM.
Regards,
Lucas
Signature · 4 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
Quoted 26 lines
Quoted 9 lines
Quoted 8 lines · Alison Wang
Quoted 6 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU intervention.
Quoted 4 lines
Quoted 2 lines · s.hauer at pengutronix.de
Maybe the real question is whether we want to introduce another framebuffer driver at all instead of making it a DRM driver.
Am Montag, den 06.01.2014, 13:50 -0500 schrieb Bill Pringlemeir:
Quoted 10 lines
I didn't understand this comment at first. I thought that the DRM infra-structure had changed or something. I see a recent post on IMX-DRM and I think there is a mis-conception on the Vybrid SOC.
At first, Freescale was to incorporate a Vivante GC355 GPU for OpenVG. However, this was removed from the design and is only present on some 'Automotive' parts, and not the VF610 nor the Tower boards. These SOCs only have a multi-level framebuffer with alpha blending. Was it meant that this be part of the DRM? It seems that the hardware without the 'Vivante GC355 GPU' is best served by an fb driver.
Quoted 3 lines · l.stach at pengutronix.de
Exactly the multi-level part of the hardware should preferably be programmed through the standardized plane stuff in the KMS userspace interface, which is part of DRM.
Ok, that makes sense. The IMX25 also has the double frame buffer and is part of the IMX family but was not converted, so that mis-lead me. I guess it is/was grand-fathered. The DRM API (GEM document, etc) for DRM seem to lead to acceleration. I will look at the KMS stuff. Is there a sample driver where multiple alpha-blended buffers are implemented?
I guess 'git grep dumb_create'? The directory 'gpu' is misleading. All of the current drivers look like they are GPU driven? Is this something that no one has ever done before?
Thanks,
Bill Pringlemeir.
PATCH 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
Quoted 3 lines · bpringlemeir at nbsps.com
I guess 'git grep
dumb_create'? The directory 'gpu' is misleading. All of the current drivers look like they are GPU driven? Is this something that no one has ever done before?
It looks like the Armada driver is the best match?
Thanks,
Bill Pringlemeir.
PATCH 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
In reply to Alison WangAm Freitag, den 12.07.2013, 14:07 +0800 schrieb Alison Wang:
Quoted 29 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU intervention.
The features:
(1) Full RGB888 output to TFT LCD panel.
(2) For the current LCD panel, WQVGA "480x272" is tested.
(3) Blending of each pixel using up to 4 source layers dependent on size of panel.
(4) Each graphic layer can be placed with one pixel resolution in either axis.
(5) Each graphic layer support RGB565 and RGB888 direct colors without alpha channel and BGRA8888 direct colors with an alpha channel.
(6) Each graphic layer support alpha blending with 8-bit resolution.This driver has been tested on Vybrid VF610 TOWER board.
Signed-off-by Alison Wang <[email protected]>---
Changes in v2: Nonedrivers/video/Kconfig | 9 + drivers/video/Makefile | 1 + drivers/video/fsl-dcu-fb.c | 1091 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1101 insertions(+) create mode 100644 drivers/video/fsl-dcu-fb.c
[...]
Quoted 8 lines
drivers/video/fsl-dcu-fb.c new file +8
-0+ +static struct fb_videomode dcu_mode_db[] = { + { + .name ="480x272" , + .refresh =75 , + .xres =480 , + .yres =272 , + .pixclock =91996 ,
drivers/video/fsl-dcu-fb.c line 156
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+ .left_margin = 2, + .right_margin = 2, + .upper_margin = 1, + .lower_margin = 1, + .hsync_len = 41, + .vsync_len = 2, + .sync = FB_SYNC_COMP_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT, + .vmode = FB_VMODE_NONINTERLACED, + }, +};
Don't hardcode panel data in the driver. Use the videomode helpers to get the relevant data from devicetree.
Quoted 22 lines
drivers/video/fsl-dcu-fb.c new file +22
-0+ +/* DCU framebuffer data structure */ +struct dcu_fb_data { +struct fb_info *fsl_dcu_info[DCU_LAYER_NUM ]; +void __iomem *reg_base; +unsigned int irq; +struct clk *clk; +}; + +struct layer_display_offset { +int x_layer_d; +int y_layer_d; +}; + +struct mfb_info { +int index; +char *id; +unsigned long pseudo_palette[16 ]; +unsigned char alpha; +unsigned char blend; +unsigned int count; +int x_layer_d;/* layer display x offset to physical screen */
drivers/video/fsl-dcu-fb.c line 188
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+int y_layer_d;/* layer display y offset to physical screen */ +struct dcu_fb_data *parent; +}; + +enum mfb_index { +LAYER0 =0 , +LAYER1 , +LAYER2 , +LAYER3 , +};
Why are there only 4 layers here? I thought the controller supports at least 6 simultaneous layers?
Quoted 39 lines
drivers/video/fsl-dcu-fb.c new file +39
-0+ +static struct mfb_info mfb_template[] = { + { + .index =LAYER0 , + .id ="Layer0" , + .alpha =0xff , + .blend =0 , + .count =0 , + .x_layer_d =0 , + .y_layer_d =0 , + }, + { + .index =LAYER1 , + .id ="Layer1" , + .alpha =0xff , + .blend =0 , + .count =0 , + .x_layer_d =50 , + .y_layer_d =50 , + }, + { + .index =LAYER2 , + .id ="Layer2" , + .alpha =0xff , + .blend =0 , + .count =0 , + .x_layer_d =100 , + .y_layer_d =100 , + }, + { + .index =LAYER3 , + .id ="Layer3" , + .alpha =0xff , + .blend =0 , + .count =0 , + .x_layer_d =150 , + .y_layer_d =150 , + }, +};
[...]
Quoted 5 lines
drivers/video/fsl-dcu-fb.c new file +5
-0+ +static int calc_div_ratio (struct fb_info *info) +{ +struct mfb_info *mfbi = info->par; +struct dcu_fb_data *dcufb = mfbi->parent;
drivers/video/fsl-dcu-fb.c line 442
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+unsigned long dcu_clk; +unsigned long long tmp; + + dcu_clk =clk_get_rate (dcufb->clk); + tmp = info->var.pixclock * (unsigned long long )dcu_clk; + +do_div (tmp,1000000 ); + +if (do_div (tmp,1000000 ) >500000 ) + tmp++;
Urgh, you are changing the value of tmp inside the if clause. This isn't nice. This function as a a whole looks really odd.
Quoted 43 lines
drivers/video/fsl-dcu-fb.c new file +43
-0+ + tmp = tmp -1 ; +return tmp; +} + +static void update_controller (struct fb_info *info) +{ +struct fb_var_screeninfo *var = &info->var; +struct mfb_info *mfbi = info->par; +struct dcu_fb_data *dcufb = mfbi->parent; +unsigned int ratio; + + ratio =calc_div_ratio (info); +writel (ratio, dcufb->reg_base +DCU_DIV_RATIO ); + +writel (DCU_DISP_SIZE_DELTA_Y (var->yres) | +DCU_DISP_SIZE_DELTA_X (var->xres /16 ), + dcufb->reg_base +DCU_DISP_SIZE ); + +/* Horizontal and vertical sync parameter */ +writel (DCU_HSYN_PARA_BP (var->left_margin) | +DCU_HSYN_PARA_PW (var->hsync_len) | +DCU_HSYN_PARA_FP (var->right_margin), + dcufb->reg_base +DCU_HSYN_PARA ); + +writel (DCU_VSYN_PARA_BP (var->upper_margin) | +DCU_VSYN_PARA_PW (var->vsync_len) | +DCU_VSYN_PARA_FP (var->lower_margin), + dcufb->reg_base +DCU_VSYN_PARA ); + +writel (DCU_SYN_POL_INV_PXCK_FALL |DCU_SYN_POL_NEG_REMAIN | +DCU_SYN_POL_INV_VS_LOW |DCU_SYN_POL_INV_HS_LOW , + dcufb->reg_base +DCU_SYN_POL ); + +writel (DCU_BGND_R (0 ) |DCU_BGND_G (0 ) |DCU_BGND_B (0 ), + dcufb->reg_base +DCU_BGND ); + +writel (DCU_MODE_BLEND_ITER (DCU_LAYER_NUM_MAX ) |DCU_MODE_RASTER_EN , + dcufb->reg_base +DCU_DCU_MODE ); + +writel (DCU_THRESHOLD_LS_BF_VS (0x3 ) |DCU_THRESHOLD_OUT_BUF_HIGH (0x78 ) | +DCU_THRESHOLD_OUT_BUF_LOW (0 ), dcufb->reg_base +DCU_THRESHOLD ); +
drivers/video/fsl-dcu-fb.c line 495
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+enable_controller (info); +} + +static int map_video_memory (struct fb_info *info) +{ +u32 smem_len = info->fix.line_length * info->var.yres_virtual; + + info->fix.smem_len = smem_len; + + info->screen_base = dma_alloc_coherent(info->device, info->fix.smem_len,
You are setting up an uncached mapping here. Use a writecombined mapping to help performance. It's still coherent, but allows at least write to be fast.
Quoted 23 lines
drivers/video/fsl-dcu-fb.c new file +23
-0+ (dma_addr_t *)&info->fix.smem_start, GFP_KERNEL); +if (!info->screen_base) { +printk (KERN_ERR "unable to allocate fb memory\n" ); +return -ENOMEM ; + } + +memset (info->screen_base,0 , info->fix.smem_len); + +return 0 ; +} + +static void unmap_video_memory (struct fb_info *info) +{ +if (!info->screen_base) +return ; + +dma_free_coherent (info->device, info->fix.smem_len, + info->screen_base, info->fix.smem_start); + + info->screen_base =NULL ; + info->fix.smem_start =0 ; + info->fix.smem_len =0 ; +}
[...]
Quoted 25 lines
drivers/video/fsl-dcu-fb.c new file +25
-0+ +static int fsl_dcu_open (struct fb_info *info,int user) +{ +struct mfb_info *mfbi = info->par; +int ret =0 ; + + mfbi->index = info->node; + + mfbi->count++; +if (mfbi->count ==1 ) { +fsl_dcu_check_var (&info->var, info); + ret =fsl_dcu_set_par (info); +if (ret <0 ) + mfbi->count--; +else +enable_interrupts (mfbi->parent); + } + +return ret; +} + +static int fsl_dcu_release (struct fb_info *info,int user) +{ +struct mfb_info *mfbi = info->par; +int ret =0 ;
drivers/video/fsl-dcu-fb.c line 764
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+ + mfbi->count--; +if (mfbi->count ==0 ) { + ret =disable_panel (info); +if (ret <0 ) + mfbi->count++; + } + +return ret; +}
Could this be replaced by runtime pm?
Quoted 98 lines
drivers/video/fsl-dcu-fb.c new file +98
-0+ +static struct fb_ops fsl_dcu_ops = { + .owner =THIS_MODULE , + .fb_check_var = fsl_dcu_check_var, + .fb_set_par = fsl_dcu_set_par, + .fb_setcolreg = fsl_dcu_setcolreg, + .fb_blank = fsl_dcu_blank, + .fb_pan_display = fsl_dcu_pan_display, + .fb_fillrect = cfb_fillrect, + .fb_copyarea = cfb_copyarea, + .fb_imageblit = cfb_imageblit, + .fb_ioctl = fsl_dcu_ioctl, + .fb_open = fsl_dcu_open, + .fb_release = fsl_dcu_release, +}; + +static int install_framebuffer (struct fb_info *info) +{ +struct mfb_info *mfbi = info->par; +struct fb_videomode *db = dcu_mode_db; +unsigned int dbsize =ARRAY_SIZE (dcu_mode_db); +int ret; + + info->var.activate =FB_ACTIVATE_NOW ; + info->fbops = &fsl_dcu_ops; + info->flags =FBINFO_FLAG_DEFAULT ; + info->pseudo_palette = &mfbi->pseudo_palette; + +fb_alloc_cmap (&info->cmap,16 ,0 ); + + ret =fb_find_mode (&info->var, info, fb_mode, db, dbsize, +NULL , default_bpp); + +if (fsl_dcu_check_var (&info->var, info)) { + ret = -EINVAL ; + goto failed_checkvar; + } + +if (register_framebuffer (info) <0 ) { + ret = -EINVAL ; + goto failed_register_framebuffer; + } + +printk (KERN_INFO "fb%d: %s fb device registered successfully.\n" , + info->node, info->fix.id); +return 0 ; + +failed_checkvar: +fb_dealloc_cmap (&info->cmap); +failed_register_framebuffer: +unmap_video_memory (info); +fb_dealloc_cmap (&info->cmap); +return ret; +} + +static void uninstall_framebuffer (struct fb_info *info) +{ +unregister_framebuffer (info); +unmap_video_memory (info); + +if (&info->cmap) +fb_dealloc_cmap (&info->cmap); +} + +static irqreturn_t fsl_dcu_irq (int irq,void *dev_id) +{ +struct dcu_fb_data *dcufb = dev_id; +unsigned int status =readl (dcufb->reg_base +DCU_INT_STATUS ); +u32 dcu_mode; + +if (status) { +if (status &DCU_INT_STATUS_UNDRUN ) { + dcu_mode =readl (dcufb->reg_base +DCU_DCU_MODE ); + dcu_mode &= ~DCU_MODE_DCU_MODE_MASK ; +writel (dcu_mode |DCU_MODE_DCU_MODE (DCU_MODE_OFF ), + dcufb->reg_base +DCU_DCU_MODE ); +udelay (1 ); +writel (dcu_mode |DCU_MODE_DCU_MODE (DCU_MODE_NORMAL ), + dcufb->reg_base +DCU_DCU_MODE ); + } +writel (status, dcufb->reg_base +DCU_INT_STATUS ); +return IRQ_HANDLED ; + } +return IRQ_NONE ; +} + +#ifdef CONFIG_PM +static int fsl_dcu_suspend (struct platform_device *pdev, +pm_message_t state) +{ +struct dcu_fb_data *dcufb =dev_get_drvdata (&pdev->dev); + +clk_disable_unprepare (dcufb->clk); +return 0 ; +} + +static int fsl_dcu_resume (struct platform_device *pdev) +{
drivers/video/fsl-dcu-fb.c line 872
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+struct dcu_fb_data *dcufb =dev_get_drvdata (&pdev->dev); + +clk_prepare_enable (dcufb->clk); + return 0; +} +#else +#define fsl_dcu_suspend NULL +#define fsl_dcu_resume NULL +#endif +
Could this be replaced by runtime pm?
Quoted 29 lines
drivers/video/fsl-dcu-fb.c new file +29
-0+static int bypass_tcon (struct device_node *np) +{ +struct device_node *tcon_np; +struct platform_device *tcon_pdev; +struct clk *tcon_clk; +void __iomem *tcon_reg; +int ret =0 ; + + tcon_np =of_parse_phandle (np,"tcon-controller" ,0 ); +if (!tcon_np) +return -EINVAL ; + + tcon_pdev =of_find_device_by_node (tcon_np); +if (!tcon_pdev) +return -EINVAL ; + + tcon_clk =devm_clk_get (&tcon_pdev->dev,"tcon" ); +if (IS_ERR (tcon_clk)) { + ret =PTR_ERR (tcon_clk); + goto failed_getclock; + } +clk_prepare_enable (tcon_clk); + + tcon_reg =of_iomap (tcon_np,0 ); +if (!tcon_reg) { + ret = -ENOMEM ; + goto failed_ioremap; + } +writel (TCON_BYPASS_ENABLE , tcon_reg +TCON_CTRL1 );
drivers/video/fsl-dcu-fb.c line 911
Quoted 10 lines
drivers/video/fsl-dcu-fb.c new file +10
-0+ +return 0 ; + +failed_ioremap: +clk_disable_unprepare (tcon_clk); +failed_getclock: +of_node_put (tcon_np); +return ret; +} +
Is the framebuffer driver the only user of tcon? If not you should not map the memory here, but rather use something like regmap syscon.
[...]
Regards,
Lucas
Signature · 4 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH v2 4/5 fb: Add DCU framebuffer driver for Vybrid VF610 platform
Quoted 37 lines
Am Freitag, den 12.07.2013, 14:07 +0800 schrieb Alison Wang:
Quoted 6 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU
intervention.
Quoted 6 lines
The features:
(1) Full RGB888 output to TFT LCD panel.
(2) For the current LCD panel, WQVGA "480x272" is tested.
(3) Blending of each pixel using up to 4 source layers dependent onsize of panel.
Quoted 1 line
(4) Each graphic layer can be placed with one pixel resolution in
either axis.
Quoted 1 line
(5) Each graphic layer support RGB565 and RGB888 direct colors
without
Quoted 16 lines
alpha channel and BGRA8888 direct colors with an alpha channel.
(6) Each graphic layer support alpha blending with 8-bit resolution.This driver has been tested on Vybrid VF610 TOWER board.
Signed-off-by Alison Wang <[email protected]>---
Changes in v2: Nonedrivers/video/Kconfig | 9 + drivers/video/Makefile | 1 + drivers/video/fsl-dcu-fb.c | 1091 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1101 insertions(+) create mode 100644 drivers/video/fsl-dcu-fb.c[...]
Quoted 8 lines
drivers/video/fsl-dcu-fb.c line 188
Quoted 6 lines
drivers/video/fsl-dcu-fb.c new file +6
-0+enum mfb_index { +LAYER0 =0 , +LAYER1 , +LAYER2 , +LAYER3 , +};Why are there only 4 layers here? I thought the controller supports at least 6 simultaneous layers?
[Alison Wang] I used 4 layers for the customer's requirement before. Anyway, I will change it to 6 in next version.
BTW, according to the RM, it said "Blending of each pixel using up to 6 source layers dependent on size of panel". So I think the most simultaneous layers is 6.
Quoted 41 lines
Quoted 40 lines
drivers/video/fsl-dcu-fb.c new file +39
-0+ +static struct mfb_info mfb_template[] = { + { + .index =LAYER0 , + .id ="Layer0" , + .alpha =0xff , + .blend =0 , + .count =0 , + .x_layer_d =0 , + .y_layer_d =0 , + }, + { + .index =LAYER1 , + .id ="Layer1" , + .alpha =0xff , + .blend =0 , + .count =0 , + .x_layer_d =50 , + .y_layer_d =50 , + }, + { + .index =LAYER2 , + .id ="Layer2" , + .alpha =0xff , + .blend =0 , + .count =0 , + .x_layer_d =100 , + .y_layer_d =100 , + }, + { + .index =LAYER3 , + .id ="Layer3" , + .alpha =0xff , + .blend =0 , + .count =0 , + .x_layer_d =150 , + .y_layer_d =150 , + }, +};[...]
Quoted 14 lines
drivers/video/fsl-dcu-fb.c line 764
Quoted 13 lines
drivers/video/fsl-dcu-fb.c new file +13
-0+static int fsl_dcu_release (struct fb_info *info,int user) { +struct mfb_info *mfbi = info->par; +int ret =0 ; + + mfbi->count--; +if (mfbi->count ==0 ) { + ret =disable_panel (info); +if (ret <0 ) + mfbi->count++; + } + +return ret; +}Could this be replaced by runtime pm?
[Alison Wang] Yes, I will use runtime pm.
Quoted 33 lines
[...]
Quoted 32 lines
drivers/video/fsl-dcu-fb.c new file +32
-0+static int bypass_tcon (struct device_node *np) { +struct device_node *tcon_np; +struct platform_device *tcon_pdev; +struct clk *tcon_clk; +void __iomem *tcon_reg; +int ret =0 ; + + tcon_np =of_parse_phandle (np,"tcon-controller" ,0 ); +if (!tcon_np) +return -EINVAL ; + + tcon_pdev =of_find_device_by_node (tcon_np); +if (!tcon_pdev) +return -EINVAL ; + + tcon_clk =devm_clk_get (&tcon_pdev->dev,"tcon" ); +if (IS_ERR (tcon_clk)) { + ret =PTR_ERR (tcon_clk); + goto failed_getclock; + } +clk_prepare_enable (tcon_clk); + + tcon_reg =of_iomap (tcon_np,0 ); +if (!tcon_reg) { + ret = -ENOMEM ; + goto failed_ioremap; + } +writel (TCON_BYPASS_ENABLE , tcon_reg +TCON_CTRL1 ); + +return 0 ; + +failed_ioremap:
Quoted 8 lines
drivers/video/fsl-dcu-fb.c line 911
Quoted 6 lines
drivers/video/fsl-dcu-fb.c new file +6
-0+clk_disable_unprepare (tcon_clk); +failed_getclock: +of_node_put (tcon_np); +return ret; +} +Is the framebuffer driver the only user of tcon? If not you should not map the memory here, but rather use something like regmap syscon.
[Alison Wang] Yes, the fb driver is the only user of tcon.
Thanks for your comments!
Best Regards,
Alison Wang
PATCH v2 5/5 Documentation: DT: Add DCU framebuffer driver
In reply to Alison WangThis patch adds the document for DCU framebuffer driver under Documentation/devicetree/bindings/fb/.
---
Changes in v2: New
- Add this document for DCU framebuffer driver.
.../devicetree/bindings/fb/fsl-dcu-fb.txt | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 Documentation/devicetree/bindings/fb/fsl-dcu-fb.txt
Documentation/devicetree/bindings/fb/fsl-dcu-fb.txt new file +36 -0
@@ -0,0 +1,36 @@ +* Freescale Display Control Unit (DCU) + +Required properties: +- compatible: Should be "fsl,vf610-dcu". Supported chips include + Vybrid VF610. +- reg: Address and length of the register set for DCU. +- interrupts: Should contain DCU interrupts. +- clocks: From common clock binding: handle to DCU clock. +- clock-names: From common clock binding: Shall be "dcu". +- tcon-controller: The phandle of TCON controller. + +For the DCU initialization, we read data from TCON node. +Required properties for TCON: +- compatible: Should be "fsl,vf610-tcon". Supported chips include + Vybrid VF610. +- reg: Address and length of the register set for TCON. +- clocks: From common clock binding: handle to TCON clock. +- clock-names: From common clock binding: Shall be "tcon". + +Examples: + +dcu0: dcu at 40058000 { + compatible = "fsl,vf610-dcu"; + reg = <0x40058000 0x1200>; + interrupts = <0 30 0x04>; + clocks = <&clks VF610_CLK_DCU0>; + clock-names = "dcu"; + tcon-controller = <&tcon0>; +}; + +tcon0: tcon at 4003d000 { + compatible = "fsl,vf610-tcon"; + reg = <0x4003d000 0x1000>; + clocks = <&clks VF610_CLK_TCON0>; + clock-names = "tcon"; +};
Signature · 1 line
1.8.0
v2 cover ARM: vf610: Add DCU framebuffer driver for Vybrid VF610 platform
In reply to Alison WangHi, Jean-Christophe,
Could you please help to review these patches?
Thanks a lot!
Best Regards,
Alison Wang
Quoted 65 lines
-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-
bounces at lists.infradead.org] On Behalf Of Alison Wang
Sent: Friday, July 12, 2013 2:08 PM
To: plagnioj at jcrosoft.com; tomi.valkeinen at ti.com; shawn.guo at linaro.org; Estevam Fabio-R49496; linux-fbdev at vger.kernel.org; linux-arm- kernel at lists.infradead.org
Cc: Jin Zhengxiong-R64188
Subject: [PATCH v2 0/5] ARM: vf610: Add DCU framebuffer driver for Vybrid VF610 platformThis series contain DCU framebuffer driver for Freescale Vybrid VF610 platform.
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU intervention.
The features:
(1) Full RGB888 output to TFT LCD panel.
(2) For the current LCD panel, WQVGA "480x272" is tested.
(3) Blending of each pixel using up to 4 source layers dependent on size of panel.
(4) Each graphic layer can be placed with one pixel resolution in either axis.
(5) Each graphic layer support RGB565 and RGB888 direct colors without alpha channel and BGRA8888 direct colors with an alpha channel.
(6) Each graphic layer support alpha blending with 8-bit resolution.Changes in v2:
- Add a document for DCU framebuffer driver underDocumentation/devicetree/bindings/fb/.----------------------------------------------------------------
Alison Wang (5):
ARM: dts: vf610: Add DCU and TCON nodes
ARM: dts: vf610-twr: Enable DCU and TCON devices
ARM: clk: vf610: Add DCU and TCON clock support
fb: Add DCU framebuffer driver for Vybrid VF610 platform
Documentation: DT: Add DCU framebuffer driverDocumentation/devicetree/bindings/fb/fsl-dcu-fb.txt | 36 ++++ arch/arm/boot/dts/vf610-twr.dts | 10 + arch/arm/boot/dts/vf610.dtsi | 19 +- arch/arm/mach-imx/clk-vf610.c | 5 + drivers/video/Kconfig | 9 + drivers/video/Makefile | 1 + drivers/video/fsl-dcu-fb.c | 1091 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ +++++++++++++++++++++++++ include/dt-bindings/clock/vf610-clock.h | 3 +- 8 files changed, 1172 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/fb/fsl-dcu-fb.txt create mode 100644 drivers/video/fsl-dcu-fb.c_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
v2 cover ARM: vf610: Add DCU framebuffer driver for Vybrid VF610 platform
Hi, Jean-Christophe, Tomi,
Do you have any comments for these patches?
Thanks!
Best Regards,
Alison Wang
Quoted 96 lines
-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-
bounces at lists.infradead.org] On Behalf Of Wang Huan-B18965
Sent: Friday, July 19, 2013 11:49 AM
To: plagnioj at jcrosoft.com; tomi.valkeinen at ti.com; shawn.guo at linaro.org; Estevam Fabio-R49496; linux-fbdev at vger.kernel.org; linux-arm- kernel at lists.infradead.org
Cc: Jin Zhengxiong-R64188
Subject: RE: [PATCH v2 0/5] ARM: vf610: Add DCU framebuffer driver for Vybrid VF610 platformHi, Jean-Christophe,
Could you please help to review these patches?
Thanks a lot!
Best Regards,
Alison WangQuoted 14 lines
-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-
bounces at lists.infradead.org] On Behalf Of Alison Wang
Sent: Friday, July 12, 2013 2:08 PM
To: plagnioj at jcrosoft.com; tomi.valkeinen at ti.com; shawn.guo at linaro.org; Estevam Fabio-R49496;
linux-fbdev at vger.kernel.org; linux-arm- kernel at lists.infradead.org
Cc: Jin Zhengxiong-R64188
Subject: [PATCH v2 0/5] ARM: vf610: Add DCU framebuffer driver for Vybrid VF610 platformThis series contain DCU framebuffer driver for Freescale Vybrid VF610 platform.
Quoted 6 lines
The Display Controller Unit (DCU) module is a system master that fetches graphics stored in internal or external memory and displays them on a TFT LCD panel. A wide range of panel sizes is supported and the timing of the interface signals is highly configurable. Graphics are read directly from memory and then blended in real-time, which allows for dynamic content creation with minimal CPU
intervention.
Quoted 10 lines
The features:
(1) Full RGB888 output to TFT LCD panel.
(2) For the current LCD panel, WQVGA "480x272" is tested.
(3) Blending of each pixel using up to 4 source layers dependent on size of panel.
(4) Each graphic layer can be placed with one pixel resolution in either axis.
(5) Each graphic layer support RGB565 and RGB888 direct colorswithout
Quoted 15 lines
alpha channel and BGRA8888 direct colors with an alpha channel.
(6) Each graphic layer support alpha blending with 8-bit resolution.Changes in v2:
- Add a document for DCU framebuffer driver underDocumentation/devicetree/bindings/fb/.----------------------------------------------------------------
Alison Wang (5):
ARM: dts: vf610: Add DCU and TCON nodes
ARM: dts: vf610-twr: Enable DCU and TCON devices
ARM: clk: vf610: Add DCU and TCON clock support
fb: Add DCU framebuffer driver for Vybrid VF610 platform
Documentation: DT: Add DCU framebuffer driverQuoted 8 lines
Documentation/devicetree/bindings/fb/fsl-dcu-fb.txt | 36 ++++ arch/arm/boot/dts/vf610-twr.dts | 10 + arch/arm/boot/dts/vf610.dtsi | 19 +- arch/arm/mach-imx/clk-vf610.c | 5 + drivers/video/Kconfig | 9 + drivers/video/Makefile | 1 + drivers/video/fsl-dcu-fb.c | 1091+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Quoted 8 lines
+++++++++++++++++++++++++
include/dt-bindings/clock/vf610-clock.h | 3 +- 8 files changed, 1172 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/fb/fsl-dcu-fb.txt create mode 100644 drivers/video/fsl-dcu-fb.cQuoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel