[PATCH v4 00/12] video: da8xx-fb: am335x DT support

[PATCH v4 00/12] video: da8xx-fb: am335x DT support

[Afzal Mohammed]

Hi,

This series adds DT support to da8xx-fb driver (device found on
DaVinci and AM335x SoC's). It does certain cleanup's in the process.

This series as compared to previous version uses new registration
interface for clock divider that has constraints on minimum divider
value.

This makes use of Steffen Trumtrar's v16 of display timing DT support.

Testing has been done on AM335x SoC based boards like AM335x EVM. It
has also been verified that display on DA850 EVM (non-DT boot) works
as earlier.

This series is based on v3.8-rc3,
 and is dependent on,
1. Series v16 "of: add display helper" by,
        Steffen Trumtrar <s.trumtrar@pengutronix.de>
2. Patch "da8xx: Allow use by am33xx based devices" by,
        Pantelis Antoniou <panto@antoniou-consulting.com>
3. Series v3 "video: da8xx-fb: runtime timing configuration" by,
        me (Afzal Mohammed <afzal@ti.com>)

To test this series on AM335x based boards,
1. Series v2 "ARM: dts: AM33XX: lcdc support" by,
        me (Afzal Mohammed <afzal@ti.com>),
2. Series "HWMOD fixes for AM33xx PWM submodules and device tree nodes" by,
        Philip, Avinash <avinashphilip@ti.com>
3. Series v2 "clk: divider: prepare for minimum divider" by,
        me (Afzal Mohammed <afzal@ti.com>),
4. Series v2 "ARM: AM335x: LCDC platform support" by,
        me (Afzal Mohammed <afzal@ti.com>),
would be needed.

All above dependencies along with those required for testing is available
@ git://gitorious.org/x0148406-public/linux-kernel.git tags/da8xx-fb-dt-v4

Regards
Afzal

v4: use new registration for clock divider having minimum divider
    requirement and have ifdef'ery in a better way
v3: model CCF clock divider with parent propogation if CCF selected
v2: 2 new patches - one to configure clock rate properly (12/12)and
    other to make io operations safe (1/12)


Afzal Mohammed (11):
  video: da8xx-fb: make io operations safe
  video: da8xx-fb: enable sync lost intr for v2 ip
  video: da8xx-fb: use devres
  video: da8xx-fb: ensure non-null cfg in pdata
  video: da8xx-fb: reorganize panel detection
  video: da8xx-fb: minimal dt support
  video: da8xx-fb: invoke platform callback safely
  video: da8xx-fb: obtain fb_videomode info from dt
  video: da8xx-fb: ensure pdata only for non-dt
  video: da8xx-fb: setup struct lcd_ctrl_config for dt
  video: da8xx-fb: CCF clock divider handling

Manjunathappa, Prakash (1):
  video: da8xx-fb: fix 24bpp raster configuration

 .../devicetree/bindings/video/fb-da8xx.txt         |  37 ++++
 drivers/video/da8xx-fb.c                           | 222 ++++++++++++++++-----
 2 files changed, 206 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt

-- 
1.7.12

[PATCH v4 00/12] video: da8xx-fb: am335x DT support

[Afzal Mohammed]

Hi,

This series adds DT support to da8xx-fb driver (device found on
DaVinci and AM335x SoC's). It does certain cleanup's in the process.

This series as compared to previous version uses new registration
interface for clock divider that has constraints on minimum divider
value.

This makes use of Steffen Trumtrar's v16 of display timing DT support.

Testing has been done on AM335x SoC based boards like AM335x EVM. It
has also been verified that display on DA850 EVM (non-DT boot) works
as earlier.

This series is based on v3.8-rc3,
 and is dependent on,
1. Series v16 "of: add display helper" by,
        Steffen Trumtrar <s.trumtrar@pengutronix.de>
2. Patch "da8xx: Allow use by am33xx based devices" by,
        Pantelis Antoniou <panto@antoniou-consulting.com>
3. Series v3 "video: da8xx-fb: runtime timing configuration" by,
        me (Afzal Mohammed <afzal@ti.com>)

To test this series on AM335x based boards,
1. Series v2 "ARM: dts: AM33XX: lcdc support" by,
        me (Afzal Mohammed <afzal@ti.com>),
2. Series "HWMOD fixes for AM33xx PWM submodules and device tree nodes" by,
        Philip, Avinash <avinashphilip@ti.com>
3. Series v2 "clk: divider: prepare for minimum divider" by,
        me (Afzal Mohammed <afzal@ti.com>),
4. Series v2 "ARM: AM335x: LCDC platform support" by,
        me (Afzal Mohammed <afzal@ti.com>),
would be needed.

All above dependencies along with those required for testing is available
@ git://gitorious.org/x0148406-public/linux-kernel.git tags/da8xx-fb-dt-v4

Regards
Afzal

v4: use new registration for clock divider having minimum divider
    requirement and have ifdef'ery in a better way
v3: model CCF clock divider with parent propogation if CCF selected
v2: 2 new patches - one to configure clock rate properly (12/12)and
    other to make io operations safe (1/12)


Afzal Mohammed (11):
  video: da8xx-fb: make io operations safe
  video: da8xx-fb: enable sync lost intr for v2 ip
  video: da8xx-fb: use devres
  video: da8xx-fb: ensure non-null cfg in pdata
  video: da8xx-fb: reorganize panel detection
  video: da8xx-fb: minimal dt support
  video: da8xx-fb: invoke platform callback safely
  video: da8xx-fb: obtain fb_videomode info from dt
  video: da8xx-fb: ensure pdata only for non-dt
  video: da8xx-fb: setup struct lcd_ctrl_config for dt
  video: da8xx-fb: CCF clock divider handling

Manjunathappa, Prakash (1):
  video: da8xx-fb: fix 24bpp raster configuration

 .../devicetree/bindings/video/fb-da8xx.txt         |  37 ++++
 drivers/video/da8xx-fb.c                           | 222 ++++++++++++++++-----
 2 files changed, 206 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt

-- 
1.7.12

[PATCH v4 01/12] video: da8xx-fb: make io operations safe

[Afzal Mohammed]

Replace __raw_readl/__raw_writel with readl/writel; this driver is
reused on ARMv7 (AM335x SoC).

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---

v2: new patch

 drivers/video/da8xx-fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 720604c..35a33ca 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -141,12 +141,12 @@ static int frame_done_flag;
 
 static inline unsigned int lcdc_read(unsigned int addr)
 {
-	return (unsigned int)__raw_readl(da8xx_fb_reg_base + (addr));
+	return (unsigned int)readl(da8xx_fb_reg_base + (addr));
 }
 
 static inline void lcdc_write(unsigned int val, unsigned int addr)
 {
-	__raw_writel(val, da8xx_fb_reg_base + (addr));
+	writel(val, da8xx_fb_reg_base + (addr));
 }
 
 struct da8xx_fb_par {
-- 
1.7.12

[PATCH v4 01/12] video: da8xx-fb: make io operations safe

[Afzal Mohammed]

Replace __raw_readl/__raw_writel with readl/writel; this driver is
reused on ARMv7 (AM335x SoC).

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---

v2: new patch

 drivers/video/da8xx-fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 720604c..35a33ca 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -141,12 +141,12 @@ static int frame_done_flag;
 
 static inline unsigned int lcdc_read(unsigned int addr)
 {
-	return (unsigned int)__raw_readl(da8xx_fb_reg_base + (addr));
+	return (unsigned int)readl(da8xx_fb_reg_base + (addr));
 }
 
 static inline void lcdc_write(unsigned int val, unsigned int addr)
 {
-	__raw_writel(val, da8xx_fb_reg_base + (addr));
+	writel(val, da8xx_fb_reg_base + (addr));
 }
 
 struct da8xx_fb_par {
-- 
1.7.12

[PATCH v4 02/12] video: da8xx-fb: fix 24bpp raster configuration

[Afzal Mohammed]

From: "Manjunathappa, Prakash" <prakash.pm@ti.com>

Set only LCD_V2_TFT_24BPP_MODE bit for 24bpp and LCD_V2_TFT_24BPP_UNPACK
bit along with LCD_V2_TFT_24BPP_MODE for 32bpp configuration.

Patch is tested on am335x-evm for 24bpp and da850-evm for 16bpp
configurations.

Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 35a33ca..7f92f37 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -550,10 +550,10 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
 	case 4:
 	case 16:
 		break;
-	case 24:
-		reg |= LCD_V2_TFT_24BPP_MODE;
 	case 32:
 		reg |= LCD_V2_TFT_24BPP_UNPACK;
+	case 24:
+		reg |= LCD_V2_TFT_24BPP_MODE;
 		break;
 
 	case 8:
-- 
1.7.12

[PATCH v4 02/12] video: da8xx-fb: fix 24bpp raster configuration

[Afzal Mohammed]

From: "Manjunathappa, Prakash" <prakash.pm-l0cyMroinI0@public.gmane.org>

Set only LCD_V2_TFT_24BPP_MODE bit for 24bpp and LCD_V2_TFT_24BPP_UNPACK
bit along with LCD_V2_TFT_24BPP_MODE for 32bpp configuration.

Patch is tested on am335x-evm for 24bpp and da850-evm for 16bpp
configurations.

Signed-off-by: Manjunathappa, Prakash <prakash.pm-l0cyMroinI0@public.gmane.org>
Signed-off-by: Afzal Mohammed <afzal-l0cyMroinI0@public.gmane.org>
---
 drivers/video/da8xx-fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 35a33ca..7f92f37 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -550,10 +550,10 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
 	case 4:
 	case 16:
 		break;
-	case 24:
-		reg |= LCD_V2_TFT_24BPP_MODE;
 	case 32:
 		reg |= LCD_V2_TFT_24BPP_UNPACK;
+	case 24:
+		reg |= LCD_V2_TFT_24BPP_MODE;
 		break;
 
 	case 8:
-- 
1.7.12

[PATCH v4 03/12] video: da8xx-fb: enable sync lost intr for v2 ip

[Afzal Mohammed]

interrupt handler is checking for sync lost interrupt, but it was not
enabled, enable it.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 7f92f37..ca69e01 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -318,7 +318,7 @@ static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
 			reg_int = lcdc_read(LCD_INT_ENABLE_SET_REG) |
 				LCD_V2_END_OF_FRAME0_INT_ENA |
 				LCD_V2_END_OF_FRAME1_INT_ENA |
-				LCD_FRAME_DONE;
+				LCD_FRAME_DONE | LCD_SYNC_LOST;
 			lcdc_write(reg_int, LCD_INT_ENABLE_SET_REG);
 		}
 		reg_dma |= LCD_DUAL_FRAME_BUFFER_ENABLE;
-- 
1.7.12

[PATCH v4 03/12] video: da8xx-fb: enable sync lost intr for v2 ip

[Afzal Mohammed]

interrupt handler is checking for sync lost interrupt, but it was not
enabled, enable it.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 7f92f37..ca69e01 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -318,7 +318,7 @@ static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
 			reg_int = lcdc_read(LCD_INT_ENABLE_SET_REG) |
 				LCD_V2_END_OF_FRAME0_INT_ENA |
 				LCD_V2_END_OF_FRAME1_INT_ENA |
-				LCD_FRAME_DONE;
+				LCD_FRAME_DONE | LCD_SYNC_LOST;
 			lcdc_write(reg_int, LCD_INT_ENABLE_SET_REG);
 		}
 		reg_dma |= LCD_DUAL_FRAME_BUFFER_ENABLE;
-- 
1.7.12

[PATCH v4 04/12] video: da8xx-fb: use devres

[Afzal Mohammed]

Replace existing resource handling in the driver with managed device
resource.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index ca69e01..7a32e83 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1036,12 +1036,9 @@ static int fb_remove(struct platform_device *dev)
 				  par->p_palette_base);
 		dma_free_coherent(NULL, par->vram_size, par->vram_virt,
 				  par->vram_phys);
-		free_irq(par->irq, par);
 		pm_runtime_put_sync(&dev->dev);
 		pm_runtime_disable(&dev->dev);
 		framebuffer_release(info);
-		iounmap(da8xx_fb_reg_base);
-		release_mem_region(lcdc_regs->start, resource_size(lcdc_regs));
 
 	}
 	return 0;
@@ -1265,7 +1262,6 @@ static int fb_probe(struct platform_device *device)
 	struct fb_info *da8xx_fb_info;
 	struct clk *fb_clk = NULL;
 	struct da8xx_fb_par *par;
-	resource_size_t len;
 	int ret, i;
 	unsigned long ulcm;
 
@@ -1275,29 +1271,16 @@ static int fb_probe(struct platform_device *device)
 	}
 
 	lcdc_regs = platform_get_resource(device, IORESOURCE_MEM, 0);
-	if (!lcdc_regs) {
-		dev_err(&device->dev,
-			"Can not get memory resource for LCD controller\n");
-		return -ENOENT;
-	}
-
-	len = resource_size(lcdc_regs);
-
-	lcdc_regs = request_mem_region(lcdc_regs->start, len, lcdc_regs->name);
-	if (!lcdc_regs)
-		return -EBUSY;
-
-	da8xx_fb_reg_base = ioremap(lcdc_regs->start, len);
+	da8xx_fb_reg_base = devm_request_and_ioremap(&device->dev, lcdc_regs);
 	if (!da8xx_fb_reg_base) {
-		ret = -EBUSY;
-		goto err_request_mem;
+		dev_err(&device->dev, "memory resource setup failed\n");
+		return -EADDRNOTAVAIL;
 	}
 
-	fb_clk = clk_get(&device->dev, "fck");
+	fb_clk = devm_clk_get(&device->dev, "fck");
 	if (IS_ERR(fb_clk)) {
 		dev_err(&device->dev, "Can not get device clock\n");
-		ret = -ENODEV;
-		goto err_ioremap;
+		return -ENODEV;
 	}
 
 	pm_runtime_enable(&device->dev);
@@ -1458,7 +1441,7 @@ static int fb_probe(struct platform_device *device)
 		lcdc_irq_handler = lcdc_irq_handler_rev02;
 	}
 
-	ret = request_irq(par->irq, lcdc_irq_handler, 0,
+	ret = devm_request_irq(&device->dev, par->irq, lcdc_irq_handler, 0,
 			DRIVER_NAME, par);
 	if (ret)
 		goto irq_freq;
@@ -1488,12 +1471,6 @@ err_pm_runtime_disable:
 	pm_runtime_put_sync(&device->dev);
 	pm_runtime_disable(&device->dev);
 
-err_ioremap:
-	iounmap(da8xx_fb_reg_base);
-
-err_request_mem:
-	release_mem_region(lcdc_regs->start, len);
-
 	return ret;
 }
 
-- 
1.7.12

[PATCH v4 04/12] video: da8xx-fb: use devres

[Afzal Mohammed]

Replace existing resource handling in the driver with managed device
resource.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index ca69e01..7a32e83 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1036,12 +1036,9 @@ static int fb_remove(struct platform_device *dev)
 				  par->p_palette_base);
 		dma_free_coherent(NULL, par->vram_size, par->vram_virt,
 				  par->vram_phys);
-		free_irq(par->irq, par);
 		pm_runtime_put_sync(&dev->dev);
 		pm_runtime_disable(&dev->dev);
 		framebuffer_release(info);
-		iounmap(da8xx_fb_reg_base);
-		release_mem_region(lcdc_regs->start, resource_size(lcdc_regs));
 
 	}
 	return 0;
@@ -1265,7 +1262,6 @@ static int fb_probe(struct platform_device *device)
 	struct fb_info *da8xx_fb_info;
 	struct clk *fb_clk = NULL;
 	struct da8xx_fb_par *par;
-	resource_size_t len;
 	int ret, i;
 	unsigned long ulcm;
 
@@ -1275,29 +1271,16 @@ static int fb_probe(struct platform_device *device)
 	}
 
 	lcdc_regs = platform_get_resource(device, IORESOURCE_MEM, 0);
-	if (!lcdc_regs) {
-		dev_err(&device->dev,
-			"Can not get memory resource for LCD controller\n");
-		return -ENOENT;
-	}
-
-	len = resource_size(lcdc_regs);
-
-	lcdc_regs = request_mem_region(lcdc_regs->start, len, lcdc_regs->name);
-	if (!lcdc_regs)
-		return -EBUSY;
-
-	da8xx_fb_reg_base = ioremap(lcdc_regs->start, len);
+	da8xx_fb_reg_base = devm_request_and_ioremap(&device->dev, lcdc_regs);
 	if (!da8xx_fb_reg_base) {
-		ret = -EBUSY;
-		goto err_request_mem;
+		dev_err(&device->dev, "memory resource setup failed\n");
+		return -EADDRNOTAVAIL;
 	}
 
-	fb_clk = clk_get(&device->dev, "fck");
+	fb_clk = devm_clk_get(&device->dev, "fck");
 	if (IS_ERR(fb_clk)) {
 		dev_err(&device->dev, "Can not get device clock\n");
-		ret = -ENODEV;
-		goto err_ioremap;
+		return -ENODEV;
 	}
 
 	pm_runtime_enable(&device->dev);
@@ -1458,7 +1441,7 @@ static int fb_probe(struct platform_device *device)
 		lcdc_irq_handler = lcdc_irq_handler_rev02;
 	}
 
-	ret = request_irq(par->irq, lcdc_irq_handler, 0,
+	ret = devm_request_irq(&device->dev, par->irq, lcdc_irq_handler, 0,
 			DRIVER_NAME, par);
 	if (ret)
 		goto irq_freq;
@@ -1488,12 +1471,6 @@ err_pm_runtime_disable:
 	pm_runtime_put_sync(&device->dev);
 	pm_runtime_disable(&device->dev);
 
-err_ioremap:
-	iounmap(da8xx_fb_reg_base);
-
-err_request_mem:
-	release_mem_region(lcdc_regs->start, len);
-
 	return ret;
 }
 
-- 
1.7.12

[PATCH v4 05/12] video: da8xx-fb: ensure non-null cfg in pdata

[Afzal Mohammed]

Ensure that platform data contains pointer for lcd_ctrl_config.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 7a32e83..3b146bc 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1320,6 +1320,11 @@ static int fb_probe(struct platform_device *device)
 
 	lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
 
+	if (!lcd_cfg) {
+		ret = -EINVAL;
+		goto err_pm_runtime_disable;
+	}
+
 	da8xx_fb_info = framebuffer_alloc(sizeof(struct da8xx_fb_par),
 					&device->dev);
 	if (!da8xx_fb_info) {
-- 
1.7.12

[PATCH v4 05/12] video: da8xx-fb: ensure non-null cfg in pdata

[Afzal Mohammed]

Ensure that platform data contains pointer for lcd_ctrl_config.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 7a32e83..3b146bc 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1320,6 +1320,11 @@ static int fb_probe(struct platform_device *device)
 
 	lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
 
+	if (!lcd_cfg) {
+		ret = -EINVAL;
+		goto err_pm_runtime_disable;
+	}
+
 	da8xx_fb_info = framebuffer_alloc(sizeof(struct da8xx_fb_par),
 					&device->dev);
 	if (!da8xx_fb_info) {
-- 
1.7.12

[PATCH v4 06/12] video: da8xx-fb: reorganize panel detection

[Afzal Mohammed]

Move panel detection to a separate function, this helps in readability
as well as makes DT support cleaner.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 3b146bc..b6ea5e9 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1253,6 +1253,27 @@ static struct fb_ops da8xx_fb_ops = {
 	.fb_blank = cfb_blank,
 };
 
+static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
+{
+	struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
+	struct fb_videomode *lcdc_info;
+	int i;
+
+	for (i = 0, lcdc_info = known_lcd_panels;
+		i < ARRAY_SIZE(known_lcd_panels); i++, lcdc_info++) {
+		if (strcmp(fb_pdata->type, lcdc_info->name) == 0)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(known_lcd_panels)) {
+		dev_err(&dev->dev, "no panel found\n");
+		return NULL;
+	}
+	dev_info(&dev->dev, "found %s panel\n", lcdc_info->name);
+
+	return lcdc_info;
+}
+
 static int fb_probe(struct platform_device *device)
 {
 	struct da8xx_lcdc_platform_data *fb_pdata =
@@ -1262,7 +1283,7 @@ static int fb_probe(struct platform_device *device)
 	struct fb_info *da8xx_fb_info;
 	struct clk *fb_clk = NULL;
 	struct da8xx_fb_par *par;
-	int ret, i;
+	int ret;
 	unsigned long ulcm;
 
 	if (fb_pdata == NULL) {
@@ -1270,6 +1291,10 @@ static int fb_probe(struct platform_device *device)
 		return -ENOENT;
 	}
 
+	lcdc_info = da8xx_fb_get_videomode(device);
+	if (lcdc_info == NULL)
+		return -ENODEV;
+
 	lcdc_regs = platform_get_resource(device, IORESOURCE_MEM, 0);
 	da8xx_fb_reg_base = devm_request_and_ioremap(&device->dev, lcdc_regs);
 	if (!da8xx_fb_reg_base) {
@@ -1303,21 +1328,6 @@ static int fb_probe(struct platform_device *device)
 		break;
 	}
 
-	for (i = 0, lcdc_info = known_lcd_panels;
-		i < ARRAY_SIZE(known_lcd_panels);
-		i++, lcdc_info++) {
-		if (strcmp(fb_pdata->type, lcdc_info->name) == 0)
-			break;
-	}
-
-	if (i == ARRAY_SIZE(known_lcd_panels)) {
-		dev_err(&device->dev, "GLCD: No valid panel found\n");
-		ret = -ENODEV;
-		goto err_pm_runtime_disable;
-	} else
-		dev_info(&device->dev, "GLCD: Found %s panel\n",
-					fb_pdata->type);
-
 	lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
 
 	if (!lcd_cfg) {
-- 
1.7.12

[PATCH v4 06/12] video: da8xx-fb: reorganize panel detection

[Afzal Mohammed]

Move panel detection to a separate function, this helps in readability
as well as makes DT support cleaner.

Signed-off-by: Afzal Mohammed <afzal-l0cyMroinI0@public.gmane.org>
---
 drivers/video/da8xx-fb.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 3b146bc..b6ea5e9 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1253,6 +1253,27 @@ static struct fb_ops da8xx_fb_ops = {
 	.fb_blank = cfb_blank,
 };
 
+static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
+{
+	struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
+	struct fb_videomode *lcdc_info;
+	int i;
+
+	for (i = 0, lcdc_info = known_lcd_panels;
+		i < ARRAY_SIZE(known_lcd_panels); i++, lcdc_info++) {
+		if (strcmp(fb_pdata->type, lcdc_info->name) == 0)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(known_lcd_panels)) {
+		dev_err(&dev->dev, "no panel found\n");
+		return NULL;
+	}
+	dev_info(&dev->dev, "found %s panel\n", lcdc_info->name);
+
+	return lcdc_info;
+}
+
 static int fb_probe(struct platform_device *device)
 {
 	struct da8xx_lcdc_platform_data *fb_pdata =
@@ -1262,7 +1283,7 @@ static int fb_probe(struct platform_device *device)
 	struct fb_info *da8xx_fb_info;
 	struct clk *fb_clk = NULL;
 	struct da8xx_fb_par *par;
-	int ret, i;
+	int ret;
 	unsigned long ulcm;
 
 	if (fb_pdata == NULL) {
@@ -1270,6 +1291,10 @@ static int fb_probe(struct platform_device *device)
 		return -ENOENT;
 	}
 
+	lcdc_info = da8xx_fb_get_videomode(device);
+	if (lcdc_info == NULL)
+		return -ENODEV;
+
 	lcdc_regs = platform_get_resource(device, IORESOURCE_MEM, 0);
 	da8xx_fb_reg_base = devm_request_and_ioremap(&device->dev, lcdc_regs);
 	if (!da8xx_fb_reg_base) {
@@ -1303,21 +1328,6 @@ static int fb_probe(struct platform_device *device)
 		break;
 	}
 
-	for (i = 0, lcdc_info = known_lcd_panels;
-		i < ARRAY_SIZE(known_lcd_panels);
-		i++, lcdc_info++) {
-		if (strcmp(fb_pdata->type, lcdc_info->name) == 0)
-			break;
-	}
-
-	if (i == ARRAY_SIZE(known_lcd_panels)) {
-		dev_err(&device->dev, "GLCD: No valid panel found\n");
-		ret = -ENODEV;
-		goto err_pm_runtime_disable;
-	} else
-		dev_info(&device->dev, "GLCD: Found %s panel\n",
-					fb_pdata->type);
-
 	lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
 
 	if (!lcd_cfg) {
-- 
1.7.12

[PATCH v4 07/12] video: da8xx-fb: minimal dt support

[Afzal Mohammed]

Driver is provided a means to have the probe triggered by DT.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 Documentation/devicetree/bindings/video/fb-da8xx.txt | 16 ++++++++++++++++
 drivers/video/da8xx-fb.c                             |  7 +++++++
 2 files changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt

diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
new file mode 100644
index 0000000..581e014
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
@@ -0,0 +1,16 @@
+TI LCD Controller on DA830/DA850/AM335x SoC's
+
+Required properties:
+- compatible:
+	DA830 - "ti,da830-lcdc"
+	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
+- reg: Address range of lcdc register set
+- interrupts: lcdc interrupt
+
+Example:
+
+lcdc@4830e000 {
+	compatible = "ti,am3352-lcdc", "ti,da830-lcdc";
+	reg =  <0x4830e000 0x1000>;
+	interrupts = <36>;
+};
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index b6ea5e9..08ee8eb 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1595,6 +1595,12 @@ static int fb_resume(struct platform_device *dev)
 #define fb_resume NULL
 #endif
 
+static const struct of_device_id da8xx_fb_of_match[] = {
+	{.compatible = "ti,da830-lcdc", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, da8xx_fb_of_match);
+
 static struct platform_driver da8xx_fb_driver = {
 	.probe = fb_probe,
 	.remove = fb_remove,
@@ -1603,6 +1609,7 @@ static struct platform_driver da8xx_fb_driver = {
 	.driver = {
 		   .name = DRIVER_NAME,
 		   .owner = THIS_MODULE,
+		   .of_match_table = of_match_ptr(da8xx_fb_of_match),
 		   },
 };
 
-- 
1.7.12

[PATCH v4 07/12] video: da8xx-fb: minimal dt support

[Afzal Mohammed]

Driver is provided a means to have the probe triggered by DT.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 Documentation/devicetree/bindings/video/fb-da8xx.txt | 16 ++++++++++++++++
 drivers/video/da8xx-fb.c                             |  7 +++++++
 2 files changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt

diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
new file mode 100644
index 0000000..581e014
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
@@ -0,0 +1,16 @@
+TI LCD Controller on DA830/DA850/AM335x SoC's
+
+Required properties:
+- compatible:
+	DA830 - "ti,da830-lcdc"
+	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
+- reg: Address range of lcdc register set
+- interrupts: lcdc interrupt
+
+Example:
+
+lcdc@4830e000 {
+	compatible = "ti,am3352-lcdc", "ti,da830-lcdc";
+	reg =  <0x4830e000 0x1000>;
+	interrupts = <36>;
+};
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index b6ea5e9..08ee8eb 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1595,6 +1595,12 @@ static int fb_resume(struct platform_device *dev)
 #define fb_resume NULL
 #endif
 
+static const struct of_device_id da8xx_fb_of_match[] = {
+	{.compatible = "ti,da830-lcdc", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, da8xx_fb_of_match);
+
 static struct platform_driver da8xx_fb_driver = {
 	.probe = fb_probe,
 	.remove = fb_remove,
@@ -1603,6 +1609,7 @@ static struct platform_driver da8xx_fb_driver = {
 	.driver = {
 		   .name = DRIVER_NAME,
 		   .owner = THIS_MODULE,
+		   .of_match_table = of_match_ptr(da8xx_fb_of_match),
 		   },
 };
 
-- 
1.7.12

[PATCH v4 08/12] video: da8xx-fb: invoke platform callback safely

[Afzal Mohammed]

Ensure that platform data is present before checking whether platform
callback is present (the one used to control backlight). So far this
was not an issue as driver was purely non-DT triggered, but now DT
support has been added.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 08ee8eb..0beed20 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1347,7 +1347,7 @@ static int fb_probe(struct platform_device *device)
 	par->dev = &device->dev;
 	par->lcdc_clk = fb_clk;
 	par->lcd_fck_rate = clk_get_rate(fb_clk);
-	if (fb_pdata->panel_power_ctrl) {
+	if (fb_pdata && fb_pdata->panel_power_ctrl) {
 		par->panel_power_ctrl = fb_pdata->panel_power_ctrl;
 		par->panel_power_ctrl(1);
 	}
-- 
1.7.12

[PATCH v4 08/12] video: da8xx-fb: invoke platform callback safely

[Afzal Mohammed]

Ensure that platform data is present before checking whether platform
callback is present (the one used to control backlight). So far this
was not an issue as driver was purely non-DT triggered, but now DT
support has been added.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 08ee8eb..0beed20 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1347,7 +1347,7 @@ static int fb_probe(struct platform_device *device)
 	par->dev = &device->dev;
 	par->lcdc_clk = fb_clk;
 	par->lcd_fck_rate = clk_get_rate(fb_clk);
-	if (fb_pdata->panel_power_ctrl) {
+	if (fb_pdata && fb_pdata->panel_power_ctrl) {
 		par->panel_power_ctrl = fb_pdata->panel_power_ctrl;
 		par->panel_power_ctrl(1);
 	}
-- 
1.7.12

[PATCH v4 09/12] video: da8xx-fb: obtain fb_videomode info from dt

[Afzal Mohammed]

Obtain fb_videomode details for the connected lcd panel using the
display timing details present in DT.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 .../devicetree/bindings/video/fb-da8xx.txt          | 21 +++++++++++++++++++++
 drivers/video/da8xx-fb.c                            | 17 +++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
index 581e014..0741f78 100644
--- a/Documentation/devicetree/bindings/video/fb-da8xx.txt
+++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
@@ -6,6 +6,12 @@ Required properties:
 	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
 - reg: Address range of lcdc register set
 - interrupts: lcdc interrupt
+- display-timings: typical videomode of lcd panel, represented as child.
+  Refer Documentation/devicetree/bindings/video/display-timing.txt for
+  display timing binding details. If multiple videomodes are mentioned
+  in display timings node, typical videomode has to be mentioned as the
+  native mode or it has to be first child (driver cares only for native
+  videomode).
 
 Example:
 
@@ -13,4 +19,19 @@ lcdc@4830e000 {
 	compatible = "ti,am3352-lcdc", "ti,da830-lcdc";
 	reg =  <0x4830e000 0x1000>;
 	interrupts = <36>;
+	display-timings {
+		800x480p62 {
+			clock-frequency = <30000000>;
+			hactive = <800>;
+			vactive = <480>;
+			hfront-porch = <39>;
+			hback-porch = <39>;
+			hsync-len = <47>;
+			vback-porch = <29>;
+			vfront-porch = <13>;
+			vsync-len = <2>;
+			hsync-active = <1>;
+			vsync-active = <1>;
+		};
+	};
 };
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 0beed20..0c68712 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -36,6 +36,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/lcm.h>
+#include <video/of_display_timing.h>
 #include <video/da8xx-fb.h>
 #include <asm/div64.h>
 
@@ -1257,8 +1258,24 @@ static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
 {
 	struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
 	struct fb_videomode *lcdc_info;
+	struct device_node *np = dev->dev.of_node;
 	int i;
 
+	if (np) {
+		lcdc_info = devm_kzalloc(&dev->dev,
+					 sizeof(struct fb_videomode),
+					 GFP_KERNEL);
+		if (!lcdc_info) {
+			dev_err(&dev->dev, "memory allocation failed\n");
+			return NULL;
+		}
+		if (of_get_fb_videomode(np, lcdc_info, OF_USE_NATIVE_MODE)) {
+			dev_err(&dev->dev, "timings not available in DT\n");
+			return NULL;
+		}
+		return lcdc_info;
+	}
+
 	for (i = 0, lcdc_info = known_lcd_panels;
 		i < ARRAY_SIZE(known_lcd_panels); i++, lcdc_info++) {
 		if (strcmp(fb_pdata->type, lcdc_info->name) == 0)
-- 
1.7.12

[PATCH v4 09/12] video: da8xx-fb: obtain fb_videomode info from dt

[Afzal Mohammed]

Obtain fb_videomode details for the connected lcd panel using the
display timing details present in DT.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 .../devicetree/bindings/video/fb-da8xx.txt          | 21 +++++++++++++++++++++
 drivers/video/da8xx-fb.c                            | 17 +++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
index 581e014..0741f78 100644
--- a/Documentation/devicetree/bindings/video/fb-da8xx.txt
+++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
@@ -6,6 +6,12 @@ Required properties:
 	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
 - reg: Address range of lcdc register set
 - interrupts: lcdc interrupt
+- display-timings: typical videomode of lcd panel, represented as child.
+  Refer Documentation/devicetree/bindings/video/display-timing.txt for
+  display timing binding details. If multiple videomodes are mentioned
+  in display timings node, typical videomode has to be mentioned as the
+  native mode or it has to be first child (driver cares only for native
+  videomode).
 
 Example:
 
@@ -13,4 +19,19 @@ lcdc@4830e000 {
 	compatible = "ti,am3352-lcdc", "ti,da830-lcdc";
 	reg =  <0x4830e000 0x1000>;
 	interrupts = <36>;
+	display-timings {
+		800x480p62 {
+			clock-frequency = <30000000>;
+			hactive = <800>;
+			vactive = <480>;
+			hfront-porch = <39>;
+			hback-porch = <39>;
+			hsync-len = <47>;
+			vback-porch = <29>;
+			vfront-porch = <13>;
+			vsync-len = <2>;
+			hsync-active = <1>;
+			vsync-active = <1>;
+		};
+	};
 };
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 0beed20..0c68712 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -36,6 +36,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/lcm.h>
+#include <video/of_display_timing.h>
 #include <video/da8xx-fb.h>
 #include <asm/div64.h>
 
@@ -1257,8 +1258,24 @@ static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
 {
 	struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
 	struct fb_videomode *lcdc_info;
+	struct device_node *np = dev->dev.of_node;
 	int i;
 
+	if (np) {
+		lcdc_info = devm_kzalloc(&dev->dev,
+					 sizeof(struct fb_videomode),
+					 GFP_KERNEL);
+		if (!lcdc_info) {
+			dev_err(&dev->dev, "memory allocation failed\n");
+			return NULL;
+		}
+		if (of_get_fb_videomode(np, lcdc_info, OF_USE_NATIVE_MODE)) {
+			dev_err(&dev->dev, "timings not available in DT\n");
+			return NULL;
+		}
+		return lcdc_info;
+	}
+
 	for (i = 0, lcdc_info = known_lcd_panels;
 		i < ARRAY_SIZE(known_lcd_panels); i++, lcdc_info++) {
 		if (strcmp(fb_pdata->type, lcdc_info->name) == 0)
-- 
1.7.12

[PATCH v4 10/12] video: da8xx-fb: ensure pdata only for non-dt

[Afzal Mohammed]

This driver is DT probe-able, hence ensure presence of platform data
only for non-DT boot.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 0c68712..1c1a616 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1303,7 +1303,7 @@ static int fb_probe(struct platform_device *device)
 	int ret;
 	unsigned long ulcm;
 
-	if (fb_pdata == NULL) {
+	if (fb_pdata == NULL && !device->dev.of_node) {
 		dev_err(&device->dev, "Can not get platform data\n");
 		return -ENOENT;
 	}
-- 
1.7.12

[PATCH v4 10/12] video: da8xx-fb: ensure pdata only for non-dt

[Afzal Mohammed]

This driver is DT probe-able, hence ensure presence of platform data
only for non-DT boot.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 0c68712..1c1a616 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1303,7 +1303,7 @@ static int fb_probe(struct platform_device *device)
 	int ret;
 	unsigned long ulcm;
 
-	if (fb_pdata == NULL) {
+	if (fb_pdata == NULL && !device->dev.of_node) {
 		dev_err(&device->dev, "Can not get platform data\n");
 		return -ENOENT;
 	}
-- 
1.7.12

[PATCH v4 11/12] video: da8xx-fb: setup struct lcd_ctrl_config for dt

[Afzal Mohammed]

strcut lcd_ctrl_config information required for driver is currently
obtained via platform data. To handle DT probing, create
lcd_ctrl_config and populate it with default values, these values are
sufficient for the panels so far used with this controller to work.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 1c1a616..5455682 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1254,6 +1254,35 @@ static struct fb_ops da8xx_fb_ops = {
 	.fb_blank = cfb_blank,
 };
 
+static struct lcd_ctrl_config *da8xx_fb_create_cfg(struct platform_device *dev)
+{
+	struct lcd_ctrl_config *cfg;
+
+	cfg = devm_kzalloc(&dev->dev, sizeof(struct fb_videomode), GFP_KERNEL);
+	if (!cfg) {
+		dev_err(&dev->dev, "memory allocation failed\n");
+		return NULL;
+	}
+
+	/* default values */
+
+	if (lcd_revision == LCD_VERSION_1)
+		cfg->bpp = 16;
+	else
+		cfg->bpp = 32;
+
+	/*
+	 * For panels so far used with this LCDC, below statement is sufficient.
+	 * For new panels, if required, struct lcd_ctrl_cfg fields to be updated
+	 * with additional/modified values. Those values would have to be then
+	 * obtained from dt(requiring new dt bindings).
+	 */
+
+	cfg->panel_shade = COLOR_ACTIVE;
+
+	return cfg;
+}
+
 static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
 {
 	struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
@@ -1345,7 +1374,10 @@ static int fb_probe(struct platform_device *device)
 		break;
 	}
 
-	lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
+	if (device->dev.of_node)
+		lcd_cfg = da8xx_fb_create_cfg(device);
+	else
+		lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
 
 	if (!lcd_cfg) {
 		ret = -EINVAL;
-- 
1.7.12

[PATCH v4 11/12] video: da8xx-fb: setup struct lcd_ctrl_config for dt

[Afzal Mohammed]

strcut lcd_ctrl_config information required for driver is currently
obtained via platform data. To handle DT probing, create
lcd_ctrl_config and populate it with default values, these values are
sufficient for the panels so far used with this controller to work.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 1c1a616..5455682 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1254,6 +1254,35 @@ static struct fb_ops da8xx_fb_ops = {
 	.fb_blank = cfb_blank,
 };
 
+static struct lcd_ctrl_config *da8xx_fb_create_cfg(struct platform_device *dev)
+{
+	struct lcd_ctrl_config *cfg;
+
+	cfg = devm_kzalloc(&dev->dev, sizeof(struct fb_videomode), GFP_KERNEL);
+	if (!cfg) {
+		dev_err(&dev->dev, "memory allocation failed\n");
+		return NULL;
+	}
+
+	/* default values */
+
+	if (lcd_revision == LCD_VERSION_1)
+		cfg->bpp = 16;
+	else
+		cfg->bpp = 32;
+
+	/*
+	 * For panels so far used with this LCDC, below statement is sufficient.
+	 * For new panels, if required, struct lcd_ctrl_cfg fields to be updated
+	 * with additional/modified values. Those values would have to be then
+	 * obtained from dt(requiring new dt bindings).
+	 */
+
+	cfg->panel_shade = COLOR_ACTIVE;
+
+	return cfg;
+}
+
 static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
 {
 	struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
@@ -1345,7 +1374,10 @@ static int fb_probe(struct platform_device *device)
 		break;
 	}
 
-	lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
+	if (device->dev.of_node)
+		lcd_cfg = da8xx_fb_create_cfg(device);
+	else
+		lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
 
 	if (!lcd_cfg) {
 		ret = -EINVAL;
-- 
1.7.12

[PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Afzal Mohammed]

Common clock framework provides a basic clock divider. Make use of it
to handle clock configuration in the LCDC IP, wherever applicable;
out of two platforms having this IP, only am335x is converted to use
CCF, DaVinci is not yet converted. Hence wrap the modification such
that it will come into effect only if CCF is selected, otherwise,
prgram dividers as earlier. Once DaVinci is converted to use CCF,
this ifdef'ery can be removed.

Divider clock instantiated is made as a one that allows the rate
propogation to it's parent, that provides more options w.r.t pixel
clock rates that could be configured.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---

v4: use new registration for clock divider having minimum divider
    requirement and have ifdef'ery in a better way
v3: model CCF clock divider with parent propogation if CCF selected
v2: new patch

 drivers/video/da8xx-fb.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 5455682..6723683 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -36,6 +36,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/lcm.h>
+#include <linux/clk-provider.h>
 #include <video/of_display_timing.h>
 #include <video/da8xx-fb.h>
 #include <asm/div64.h>
@@ -133,6 +134,10 @@
 #define WSI_TIMEOUT	50
 #define PALETTE_SIZE	256
 
+#define	LCD_CLK_SHIFT	8
+#define	LCD_CLK_WIDTH	8
+#define	LCD_CLK_MIN_DIV	2
+
 static void __iomem *da8xx_fb_reg_base;
 static struct resource *lcdc_regs;
 static unsigned int lcd_revision;
@@ -181,6 +186,9 @@ struct da8xx_fb_par {
 	u32 pseudo_palette[16];
 	struct fb_videomode	mode;
 	struct lcd_ctrl_config	cfg;
+#ifdef CONFIG_COMMON_CLK
+	struct clk		*child_clk;
+#endif
 };
 
 static struct fb_var_screeninfo da8xx_fb_var;
@@ -683,12 +691,27 @@ static void da8xx_fb_lcd_reset(void)
 	}
 }
 
+#ifndef	CONFIG_COMMON_CLK
 static inline unsigned da8xx_fb_calc_clk_divider(struct da8xx_fb_par *par,
 						 unsigned pixclock)
 {
 	return par->lcd_fck_rate / (PICOS2KHZ(pixclock) * 1000);
 }
+#endif
 
+#ifdef	CONFIG_COMMON_CLK
+static inline unsigned da8xx_fb_round_clk(struct da8xx_fb_par *par,
+					  unsigned pixclock)
+{
+	unsigned long rate;
+
+	rate = PICOS2KHZ(pixclock) * 1000;
+	rate = clk_round_rate(par->child_clk, rate);
+	rate = KHZ2PICOS(rate / 1000);
+
+	return rate;
+}
+#else
 static inline unsigned da8xx_fb_round_clk(struct da8xx_fb_par *par,
 					  unsigned pixclock)
 {
@@ -703,19 +726,43 @@ static inline void da8xx_fb_config_clk_divider(unsigned div)
 	/* Configure the LCD clock divisor. */
 	lcdc_write(LCD_CLK_DIVISOR(div) |
 			(LCD_RASTER_MODE & 0x1), LCD_CTRL_REG);
+}
+#endif
 
+static inline void da8xx_fb_clkc_enable(void)
+{
 	if (lcd_revision == LCD_VERSION_2)
 		lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
 				LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
 }
 
-static inline void da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
+#ifdef	CONFIG_COMMON_CLK
+static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
+						    struct fb_videomode *mode)
+{
+	int ret;
+
+	ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
+	if (IS_ERR_VALUE(ret)) {
+		dev_err(par->dev, "unable to setup pixel clock of %u ps",
+			mode->pixclock);
+		return ret;
+	}
+	da8xx_fb_clkc_enable();
+	return 0;
+}
+#else
+static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
 						    struct fb_videomode *mode)
 {
 	unsigned div = da8xx_fb_calc_clk_divider(par, mode->pixclock);
 
 	da8xx_fb_config_clk_divider(div);
+	da8xx_fb_clkc_enable();
+
+	return 0;
 }
+#endif
 
 static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
 		struct fb_videomode *panel)
@@ -723,7 +770,9 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
 	u32 bpp;
 	int ret = 0;
 
-	da8xx_fb_calc_config_clk_divider(par, panel);
+	ret = da8xx_fb_calc_config_clk_divider(par, panel);
+	if (IS_ERR_VALUE(ret))
+		return ret;
 
 	if (panel->sync & FB_SYNC_CLK_INVERT)
 		lcdc_write((lcdc_read(LCD_RASTER_TIMING_2_REG) |
@@ -1406,6 +1455,25 @@ static int fb_probe(struct platform_device *device)
 
 	da8xx_fb_lcd_reset();
 
+#ifdef	CONFIG_COMMON_CLK
+	/* set sane divisor value to begin along with the mode */
+	lcdc_write(LCD_RASTER_MODE | LCD_CLK_DIVISOR(LCD_CLK_MIN_DIV),
+		   LCD_CTRL_REG);
+
+	par->child_clk = clk_register_min_divider(NULL, "da8xx_fb_clk",
+					      __clk_get_name(fb_clk),
+					      CLK_SET_RATE_PARENT,
+					      da8xx_fb_reg_base + LCD_CTRL_REG,
+					      LCD_CLK_SHIFT, LCD_CLK_WIDTH,
+					      LCD_CLK_MIN_DIV,
+					      CLK_DIVIDER_ONE_BASED, NULL);
+	if (IS_ERR(par->child_clk)) {
+		dev_err(&device->dev, "error registering clk\n");
+		ret = -ENODEV;
+		goto err_release_fb;
+	}
+#endif
+
 	/* allocate frame buffer */
 	par->vram_size = lcdc_info->xres * lcdc_info->yres * lcd_cfg->bpp;
 	ulcm = lcm((lcdc_info->xres * lcd_cfg->bpp)/8, PAGE_SIZE);
-- 
1.7.12

[PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Afzal Mohammed]

Common clock framework provides a basic clock divider. Make use of it
to handle clock configuration in the LCDC IP, wherever applicable;
out of two platforms having this IP, only am335x is converted to use
CCF, DaVinci is not yet converted. Hence wrap the modification such
that it will come into effect only if CCF is selected, otherwise,
prgram dividers as earlier. Once DaVinci is converted to use CCF,
this ifdef'ery can be removed.

Divider clock instantiated is made as a one that allows the rate
propogation to it's parent, that provides more options w.r.t pixel
clock rates that could be configured.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---

v4: use new registration for clock divider having minimum divider
    requirement and have ifdef'ery in a better way
v3: model CCF clock divider with parent propogation if CCF selected
v2: new patch

 drivers/video/da8xx-fb.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 5455682..6723683 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -36,6 +36,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/lcm.h>
+#include <linux/clk-provider.h>
 #include <video/of_display_timing.h>
 #include <video/da8xx-fb.h>
 #include <asm/div64.h>
@@ -133,6 +134,10 @@
 #define WSI_TIMEOUT	50
 #define PALETTE_SIZE	256
 
+#define	LCD_CLK_SHIFT	8
+#define	LCD_CLK_WIDTH	8
+#define	LCD_CLK_MIN_DIV	2
+
 static void __iomem *da8xx_fb_reg_base;
 static struct resource *lcdc_regs;
 static unsigned int lcd_revision;
@@ -181,6 +186,9 @@ struct da8xx_fb_par {
 	u32 pseudo_palette[16];
 	struct fb_videomode	mode;
 	struct lcd_ctrl_config	cfg;
+#ifdef CONFIG_COMMON_CLK
+	struct clk		*child_clk;
+#endif
 };
 
 static struct fb_var_screeninfo da8xx_fb_var;
@@ -683,12 +691,27 @@ static void da8xx_fb_lcd_reset(void)
 	}
 }
 
+#ifndef	CONFIG_COMMON_CLK
 static inline unsigned da8xx_fb_calc_clk_divider(struct da8xx_fb_par *par,
 						 unsigned pixclock)
 {
 	return par->lcd_fck_rate / (PICOS2KHZ(pixclock) * 1000);
 }
+#endif
 
+#ifdef	CONFIG_COMMON_CLK
+static inline unsigned da8xx_fb_round_clk(struct da8xx_fb_par *par,
+					  unsigned pixclock)
+{
+	unsigned long rate;
+
+	rate = PICOS2KHZ(pixclock) * 1000;
+	rate = clk_round_rate(par->child_clk, rate);
+	rate = KHZ2PICOS(rate / 1000);
+
+	return rate;
+}
+#else
 static inline unsigned da8xx_fb_round_clk(struct da8xx_fb_par *par,
 					  unsigned pixclock)
 {
@@ -703,19 +726,43 @@ static inline void da8xx_fb_config_clk_divider(unsigned div)
 	/* Configure the LCD clock divisor. */
 	lcdc_write(LCD_CLK_DIVISOR(div) |
 			(LCD_RASTER_MODE & 0x1), LCD_CTRL_REG);
+}
+#endif
 
+static inline void da8xx_fb_clkc_enable(void)
+{
 	if (lcd_revision == LCD_VERSION_2)
 		lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
 				LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
 }
 
-static inline void da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
+#ifdef	CONFIG_COMMON_CLK
+static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
+						    struct fb_videomode *mode)
+{
+	int ret;
+
+	ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
+	if (IS_ERR_VALUE(ret)) {
+		dev_err(par->dev, "unable to setup pixel clock of %u ps",
+			mode->pixclock);
+		return ret;
+	}
+	da8xx_fb_clkc_enable();
+	return 0;
+}
+#else
+static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
 						    struct fb_videomode *mode)
 {
 	unsigned div = da8xx_fb_calc_clk_divider(par, mode->pixclock);
 
 	da8xx_fb_config_clk_divider(div);
+	da8xx_fb_clkc_enable();
+
+	return 0;
 }
+#endif
 
 static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
 		struct fb_videomode *panel)
@@ -723,7 +770,9 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
 	u32 bpp;
 	int ret = 0;
 
-	da8xx_fb_calc_config_clk_divider(par, panel);
+	ret = da8xx_fb_calc_config_clk_divider(par, panel);
+	if (IS_ERR_VALUE(ret))
+		return ret;
 
 	if (panel->sync & FB_SYNC_CLK_INVERT)
 		lcdc_write((lcdc_read(LCD_RASTER_TIMING_2_REG) |
@@ -1406,6 +1455,25 @@ static int fb_probe(struct platform_device *device)
 
 	da8xx_fb_lcd_reset();
 
+#ifdef	CONFIG_COMMON_CLK
+	/* set sane divisor value to begin along with the mode */
+	lcdc_write(LCD_RASTER_MODE | LCD_CLK_DIVISOR(LCD_CLK_MIN_DIV),
+		   LCD_CTRL_REG);
+
+	par->child_clk = clk_register_min_divider(NULL, "da8xx_fb_clk",
+					      __clk_get_name(fb_clk),
+					      CLK_SET_RATE_PARENT,
+					      da8xx_fb_reg_base + LCD_CTRL_REG,
+					      LCD_CLK_SHIFT, LCD_CLK_WIDTH,
+					      LCD_CLK_MIN_DIV,
+					      CLK_DIVIDER_ONE_BASED, NULL);
+	if (IS_ERR(par->child_clk)) {
+		dev_err(&device->dev, "error registering clk\n");
+		ret = -ENODEV;
+		goto err_release_fb;
+	}
+#endif
+
 	/* allocate frame buffer */
 	par->vram_size = lcdc_info->xres * lcdc_info->yres * lcd_cfg->bpp;
 	ulcm = lcm((lcdc_info->xres * lcd_cfg->bpp)/8, PAGE_SIZE);
-- 
1.7.12

[PATCH v4 08/12] video: da8xx-fb: invoke platform callback safely

[Afzal Mohammed]

Ensure that platform data is present before checking whether platform
callback is present (the one used to control backlight). So far this
was not an issue as driver was purely non-DT triggered, but now DT
support has been added.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 08ee8eb..0beed20 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1347,7 +1347,7 @@ static int fb_probe(struct platform_device *device)
 	par->dev = &device->dev;
 	par->lcdc_clk = fb_clk;
 	par->lcd_fck_rate = clk_get_rate(fb_clk);
-	if (fb_pdata->panel_power_ctrl) {
+	if (fb_pdata && fb_pdata->panel_power_ctrl) {
 		par->panel_power_ctrl = fb_pdata->panel_power_ctrl;
 		par->panel_power_ctrl(1);
 	}
-- 
1.7.12

[PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Afzal Mohammed]

Common clock framework provides a basic clock divider. Make use of it
to handle clock configuration in the LCDC IP, wherever applicable;
out of two platforms having this IP, only am335x is converted to use
CCF, DaVinci is not yet converted. Hence wrap the modification such
that it will come into effect only if CCF is selected, otherwise,
prgram dividers as earlier. Once DaVinci is converted to use CCF,
this ifdef'ery can be removed.

Divider clock instantiated is made as a one that allows the rate
propogation to it's parent, that provides more options w.r.t pixel
clock rates that could be configured.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---

v4: use new registration for clock divider having minimum divider
    requirement and have ifdef'ery in a better way
v3: model CCF clock divider with parent propogation if CCF selected
v2: new patch

 drivers/video/da8xx-fb.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 5455682..6723683 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -36,6 +36,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/lcm.h>
+#include <linux/clk-provider.h>
 #include <video/of_display_timing.h>
 #include <video/da8xx-fb.h>
 #include <asm/div64.h>
@@ -133,6 +134,10 @@
 #define WSI_TIMEOUT	50
 #define PALETTE_SIZE	256
 
+#define	LCD_CLK_SHIFT	8
+#define	LCD_CLK_WIDTH	8
+#define	LCD_CLK_MIN_DIV	2
+
 static void __iomem *da8xx_fb_reg_base;
 static struct resource *lcdc_regs;
 static unsigned int lcd_revision;
@@ -181,6 +186,9 @@ struct da8xx_fb_par {
 	u32 pseudo_palette[16];
 	struct fb_videomode	mode;
 	struct lcd_ctrl_config	cfg;
+#ifdef CONFIG_COMMON_CLK
+	struct clk		*child_clk;
+#endif
 };
 
 static struct fb_var_screeninfo da8xx_fb_var;
@@ -683,12 +691,27 @@ static void da8xx_fb_lcd_reset(void)
 	}
 }
 
+#ifndef	CONFIG_COMMON_CLK
 static inline unsigned da8xx_fb_calc_clk_divider(struct da8xx_fb_par *par,
 						 unsigned pixclock)
 {
 	return par->lcd_fck_rate / (PICOS2KHZ(pixclock) * 1000);
 }
+#endif
 
+#ifdef	CONFIG_COMMON_CLK
+static inline unsigned da8xx_fb_round_clk(struct da8xx_fb_par *par,
+					  unsigned pixclock)
+{
+	unsigned long rate;
+
+	rate = PICOS2KHZ(pixclock) * 1000;
+	rate = clk_round_rate(par->child_clk, rate);
+	rate = KHZ2PICOS(rate / 1000);
+
+	return rate;
+}
+#else
 static inline unsigned da8xx_fb_round_clk(struct da8xx_fb_par *par,
 					  unsigned pixclock)
 {
@@ -703,19 +726,43 @@ static inline void da8xx_fb_config_clk_divider(unsigned div)
 	/* Configure the LCD clock divisor. */
 	lcdc_write(LCD_CLK_DIVISOR(div) |
 			(LCD_RASTER_MODE & 0x1), LCD_CTRL_REG);
+}
+#endif
 
+static inline void da8xx_fb_clkc_enable(void)
+{
 	if (lcd_revision = LCD_VERSION_2)
 		lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
 				LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
 }
 
-static inline void da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
+#ifdef	CONFIG_COMMON_CLK
+static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
+						    struct fb_videomode *mode)
+{
+	int ret;
+
+	ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
+	if (IS_ERR_VALUE(ret)) {
+		dev_err(par->dev, "unable to setup pixel clock of %u ps",
+			mode->pixclock);
+		return ret;
+	}
+	da8xx_fb_clkc_enable();
+	return 0;
+}
+#else
+static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
 						    struct fb_videomode *mode)
 {
 	unsigned div = da8xx_fb_calc_clk_divider(par, mode->pixclock);
 
 	da8xx_fb_config_clk_divider(div);
+	da8xx_fb_clkc_enable();
+
+	return 0;
 }
+#endif
 
 static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
 		struct fb_videomode *panel)
@@ -723,7 +770,9 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
 	u32 bpp;
 	int ret = 0;
 
-	da8xx_fb_calc_config_clk_divider(par, panel);
+	ret = da8xx_fb_calc_config_clk_divider(par, panel);
+	if (IS_ERR_VALUE(ret))
+		return ret;
 
 	if (panel->sync & FB_SYNC_CLK_INVERT)
 		lcdc_write((lcdc_read(LCD_RASTER_TIMING_2_REG) |
@@ -1406,6 +1455,25 @@ static int fb_probe(struct platform_device *device)
 
 	da8xx_fb_lcd_reset();
 
+#ifdef	CONFIG_COMMON_CLK
+	/* set sane divisor value to begin along with the mode */
+	lcdc_write(LCD_RASTER_MODE | LCD_CLK_DIVISOR(LCD_CLK_MIN_DIV),
+		   LCD_CTRL_REG);
+
+	par->child_clk = clk_register_min_divider(NULL, "da8xx_fb_clk",
+					      __clk_get_name(fb_clk),
+					      CLK_SET_RATE_PARENT,
+					      da8xx_fb_reg_base + LCD_CTRL_REG,
+					      LCD_CLK_SHIFT, LCD_CLK_WIDTH,
+					      LCD_CLK_MIN_DIV,
+					      CLK_DIVIDER_ONE_BASED, NULL);
+	if (IS_ERR(par->child_clk)) {
+		dev_err(&device->dev, "error registering clk\n");
+		ret = -ENODEV;
+		goto err_release_fb;
+	}
+#endif
+
 	/* allocate frame buffer */
 	par->vram_size = lcdc_info->xres * lcdc_info->yres * lcd_cfg->bpp;
 	ulcm = lcm((lcdc_info->xres * lcd_cfg->bpp)/8, PAGE_SIZE);
-- 
1.7.12

[PATCH v4 11/12] video: da8xx-fb: setup struct lcd_ctrl_config for dt

[Afzal Mohammed]

strcut lcd_ctrl_config information required for driver is currently
obtained via platform data. To handle DT probing, create
lcd_ctrl_config and populate it with default values, these values are
sufficient for the panels so far used with this controller to work.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 1c1a616..5455682 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1254,6 +1254,35 @@ static struct fb_ops da8xx_fb_ops = {
 	.fb_blank = cfb_blank,
 };
 
+static struct lcd_ctrl_config *da8xx_fb_create_cfg(struct platform_device *dev)
+{
+	struct lcd_ctrl_config *cfg;
+
+	cfg = devm_kzalloc(&dev->dev, sizeof(struct fb_videomode), GFP_KERNEL);
+	if (!cfg) {
+		dev_err(&dev->dev, "memory allocation failed\n");
+		return NULL;
+	}
+
+	/* default values */
+
+	if (lcd_revision = LCD_VERSION_1)
+		cfg->bpp = 16;
+	else
+		cfg->bpp = 32;
+
+	/*
+	 * For panels so far used with this LCDC, below statement is sufficient.
+	 * For new panels, if required, struct lcd_ctrl_cfg fields to be updated
+	 * with additional/modified values. Those values would have to be then
+	 * obtained from dt(requiring new dt bindings).
+	 */
+
+	cfg->panel_shade = COLOR_ACTIVE;
+
+	return cfg;
+}
+
 static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
 {
 	struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
@@ -1345,7 +1374,10 @@ static int fb_probe(struct platform_device *device)
 		break;
 	}
 
-	lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
+	if (device->dev.of_node)
+		lcd_cfg = da8xx_fb_create_cfg(device);
+	else
+		lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
 
 	if (!lcd_cfg) {
 		ret = -EINVAL;
-- 
1.7.12

[PATCH v4 10/12] video: da8xx-fb: ensure pdata only for non-dt

[Afzal Mohammed]

This driver is DT probe-able, hence ensure presence of platform data
only for non-DT boot.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 0c68712..1c1a616 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1303,7 +1303,7 @@ static int fb_probe(struct platform_device *device)
 	int ret;
 	unsigned long ulcm;
 
-	if (fb_pdata = NULL) {
+	if (fb_pdata = NULL && !device->dev.of_node) {
 		dev_err(&device->dev, "Can not get platform data\n");
 		return -ENOENT;
 	}
-- 
1.7.12

[PATCH v4 09/12] video: da8xx-fb: obtain fb_videomode info from dt

[Afzal Mohammed]

Obtain fb_videomode details for the connected lcd panel using the
display timing details present in DT.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 .../devicetree/bindings/video/fb-da8xx.txt          | 21 +++++++++++++++++++++
 drivers/video/da8xx-fb.c                            | 17 +++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
index 581e014..0741f78 100644
--- a/Documentation/devicetree/bindings/video/fb-da8xx.txt
+++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
@@ -6,6 +6,12 @@ Required properties:
 	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
 - reg: Address range of lcdc register set
 - interrupts: lcdc interrupt
+- display-timings: typical videomode of lcd panel, represented as child.
+  Refer Documentation/devicetree/bindings/video/display-timing.txt for
+  display timing binding details. If multiple videomodes are mentioned
+  in display timings node, typical videomode has to be mentioned as the
+  native mode or it has to be first child (driver cares only for native
+  videomode).
 
 Example:
 
@@ -13,4 +19,19 @@ lcdc@4830e000 {
 	compatible = "ti,am3352-lcdc", "ti,da830-lcdc";
 	reg =  <0x4830e000 0x1000>;
 	interrupts = <36>;
+	display-timings {
+		800x480p62 {
+			clock-frequency = <30000000>;
+			hactive = <800>;
+			vactive = <480>;
+			hfront-porch = <39>;
+			hback-porch = <39>;
+			hsync-len = <47>;
+			vback-porch = <29>;
+			vfront-porch = <13>;
+			vsync-len = <2>;
+			hsync-active = <1>;
+			vsync-active = <1>;
+		};
+	};
 };
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 0beed20..0c68712 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -36,6 +36,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/lcm.h>
+#include <video/of_display_timing.h>
 #include <video/da8xx-fb.h>
 #include <asm/div64.h>
 
@@ -1257,8 +1258,24 @@ static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
 {
 	struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
 	struct fb_videomode *lcdc_info;
+	struct device_node *np = dev->dev.of_node;
 	int i;
 
+	if (np) {
+		lcdc_info = devm_kzalloc(&dev->dev,
+					 sizeof(struct fb_videomode),
+					 GFP_KERNEL);
+		if (!lcdc_info) {
+			dev_err(&dev->dev, "memory allocation failed\n");
+			return NULL;
+		}
+		if (of_get_fb_videomode(np, lcdc_info, OF_USE_NATIVE_MODE)) {
+			dev_err(&dev->dev, "timings not available in DT\n");
+			return NULL;
+		}
+		return lcdc_info;
+	}
+
 	for (i = 0, lcdc_info = known_lcd_panels;
 		i < ARRAY_SIZE(known_lcd_panels); i++, lcdc_info++) {
 		if (strcmp(fb_pdata->type, lcdc_info->name) = 0)
-- 
1.7.12

[PATCH v4 07/12] video: da8xx-fb: minimal dt support

[Afzal Mohammed]

Driver is provided a means to have the probe triggered by DT.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 Documentation/devicetree/bindings/video/fb-da8xx.txt | 16 ++++++++++++++++
 drivers/video/da8xx-fb.c                             |  7 +++++++
 2 files changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt

diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
new file mode 100644
index 0000000..581e014
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
@@ -0,0 +1,16 @@
+TI LCD Controller on DA830/DA850/AM335x SoC's
+
+Required properties:
+- compatible:
+	DA830 - "ti,da830-lcdc"
+	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
+- reg: Address range of lcdc register set
+- interrupts: lcdc interrupt
+
+Example:
+
+lcdc@4830e000 {
+	compatible = "ti,am3352-lcdc", "ti,da830-lcdc";
+	reg =  <0x4830e000 0x1000>;
+	interrupts = <36>;
+};
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index b6ea5e9..08ee8eb 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1595,6 +1595,12 @@ static int fb_resume(struct platform_device *dev)
 #define fb_resume NULL
 #endif
 
+static const struct of_device_id da8xx_fb_of_match[] = {
+	{.compatible = "ti,da830-lcdc", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, da8xx_fb_of_match);
+
 static struct platform_driver da8xx_fb_driver = {
 	.probe = fb_probe,
 	.remove = fb_remove,
@@ -1603,6 +1609,7 @@ static struct platform_driver da8xx_fb_driver = {
 	.driver = {
 		   .name = DRIVER_NAME,
 		   .owner = THIS_MODULE,
+		   .of_match_table = of_match_ptr(da8xx_fb_of_match),
 		   },
 };
 
-- 
1.7.12

[PATCH v4 06/12] video: da8xx-fb: reorganize panel detection

[Afzal Mohammed]

Move panel detection to a separate function, this helps in readability
as well as makes DT support cleaner.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 3b146bc..b6ea5e9 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1253,6 +1253,27 @@ static struct fb_ops da8xx_fb_ops = {
 	.fb_blank = cfb_blank,
 };
 
+static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
+{
+	struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
+	struct fb_videomode *lcdc_info;
+	int i;
+
+	for (i = 0, lcdc_info = known_lcd_panels;
+		i < ARRAY_SIZE(known_lcd_panels); i++, lcdc_info++) {
+		if (strcmp(fb_pdata->type, lcdc_info->name) = 0)
+			break;
+	}
+
+	if (i = ARRAY_SIZE(known_lcd_panels)) {
+		dev_err(&dev->dev, "no panel found\n");
+		return NULL;
+	}
+	dev_info(&dev->dev, "found %s panel\n", lcdc_info->name);
+
+	return lcdc_info;
+}
+
 static int fb_probe(struct platform_device *device)
 {
 	struct da8xx_lcdc_platform_data *fb_pdata @@ -1262,7 +1283,7 @@ static int fb_probe(struct platform_device *device)
 	struct fb_info *da8xx_fb_info;
 	struct clk *fb_clk = NULL;
 	struct da8xx_fb_par *par;
-	int ret, i;
+	int ret;
 	unsigned long ulcm;
 
 	if (fb_pdata = NULL) {
@@ -1270,6 +1291,10 @@ static int fb_probe(struct platform_device *device)
 		return -ENOENT;
 	}
 
+	lcdc_info = da8xx_fb_get_videomode(device);
+	if (lcdc_info = NULL)
+		return -ENODEV;
+
 	lcdc_regs = platform_get_resource(device, IORESOURCE_MEM, 0);
 	da8xx_fb_reg_base = devm_request_and_ioremap(&device->dev, lcdc_regs);
 	if (!da8xx_fb_reg_base) {
@@ -1303,21 +1328,6 @@ static int fb_probe(struct platform_device *device)
 		break;
 	}
 
-	for (i = 0, lcdc_info = known_lcd_panels;
-		i < ARRAY_SIZE(known_lcd_panels);
-		i++, lcdc_info++) {
-		if (strcmp(fb_pdata->type, lcdc_info->name) = 0)
-			break;
-	}
-
-	if (i = ARRAY_SIZE(known_lcd_panels)) {
-		dev_err(&device->dev, "GLCD: No valid panel found\n");
-		ret = -ENODEV;
-		goto err_pm_runtime_disable;
-	} else
-		dev_info(&device->dev, "GLCD: Found %s panel\n",
-					fb_pdata->type);
-
 	lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
 
 	if (!lcd_cfg) {
-- 
1.7.12

[PATCH v4 05/12] video: da8xx-fb: ensure non-null cfg in pdata

[Afzal Mohammed]

Ensure that platform data contains pointer for lcd_ctrl_config.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 7a32e83..3b146bc 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1320,6 +1320,11 @@ static int fb_probe(struct platform_device *device)
 
 	lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
 
+	if (!lcd_cfg) {
+		ret = -EINVAL;
+		goto err_pm_runtime_disable;
+	}
+
 	da8xx_fb_info = framebuffer_alloc(sizeof(struct da8xx_fb_par),
 					&device->dev);
 	if (!da8xx_fb_info) {
-- 
1.7.12

[PATCH v4 04/12] video: da8xx-fb: use devres

[Afzal Mohammed]

Replace existing resource handling in the driver with managed device
resource.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index ca69e01..7a32e83 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1036,12 +1036,9 @@ static int fb_remove(struct platform_device *dev)
 				  par->p_palette_base);
 		dma_free_coherent(NULL, par->vram_size, par->vram_virt,
 				  par->vram_phys);
-		free_irq(par->irq, par);
 		pm_runtime_put_sync(&dev->dev);
 		pm_runtime_disable(&dev->dev);
 		framebuffer_release(info);
-		iounmap(da8xx_fb_reg_base);
-		release_mem_region(lcdc_regs->start, resource_size(lcdc_regs));
 
 	}
 	return 0;
@@ -1265,7 +1262,6 @@ static int fb_probe(struct platform_device *device)
 	struct fb_info *da8xx_fb_info;
 	struct clk *fb_clk = NULL;
 	struct da8xx_fb_par *par;
-	resource_size_t len;
 	int ret, i;
 	unsigned long ulcm;
 
@@ -1275,29 +1271,16 @@ static int fb_probe(struct platform_device *device)
 	}
 
 	lcdc_regs = platform_get_resource(device, IORESOURCE_MEM, 0);
-	if (!lcdc_regs) {
-		dev_err(&device->dev,
-			"Can not get memory resource for LCD controller\n");
-		return -ENOENT;
-	}
-
-	len = resource_size(lcdc_regs);
-
-	lcdc_regs = request_mem_region(lcdc_regs->start, len, lcdc_regs->name);
-	if (!lcdc_regs)
-		return -EBUSY;
-
-	da8xx_fb_reg_base = ioremap(lcdc_regs->start, len);
+	da8xx_fb_reg_base = devm_request_and_ioremap(&device->dev, lcdc_regs);
 	if (!da8xx_fb_reg_base) {
-		ret = -EBUSY;
-		goto err_request_mem;
+		dev_err(&device->dev, "memory resource setup failed\n");
+		return -EADDRNOTAVAIL;
 	}
 
-	fb_clk = clk_get(&device->dev, "fck");
+	fb_clk = devm_clk_get(&device->dev, "fck");
 	if (IS_ERR(fb_clk)) {
 		dev_err(&device->dev, "Can not get device clock\n");
-		ret = -ENODEV;
-		goto err_ioremap;
+		return -ENODEV;
 	}
 
 	pm_runtime_enable(&device->dev);
@@ -1458,7 +1441,7 @@ static int fb_probe(struct platform_device *device)
 		lcdc_irq_handler = lcdc_irq_handler_rev02;
 	}
 
-	ret = request_irq(par->irq, lcdc_irq_handler, 0,
+	ret = devm_request_irq(&device->dev, par->irq, lcdc_irq_handler, 0,
 			DRIVER_NAME, par);
 	if (ret)
 		goto irq_freq;
@@ -1488,12 +1471,6 @@ err_pm_runtime_disable:
 	pm_runtime_put_sync(&device->dev);
 	pm_runtime_disable(&device->dev);
 
-err_ioremap:
-	iounmap(da8xx_fb_reg_base);
-
-err_request_mem:
-	release_mem_region(lcdc_regs->start, len);
-
 	return ret;
 }
 
-- 
1.7.12

[PATCH v4 03/12] video: da8xx-fb: enable sync lost intr for v2 ip

[Afzal Mohammed]

interrupt handler is checking for sync lost interrupt, but it was not
enabled, enable it.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 7f92f37..ca69e01 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -318,7 +318,7 @@ static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
 			reg_int = lcdc_read(LCD_INT_ENABLE_SET_REG) |
 				LCD_V2_END_OF_FRAME0_INT_ENA |
 				LCD_V2_END_OF_FRAME1_INT_ENA |
-				LCD_FRAME_DONE;
+				LCD_FRAME_DONE | LCD_SYNC_LOST;
 			lcdc_write(reg_int, LCD_INT_ENABLE_SET_REG);
 		}
 		reg_dma |= LCD_DUAL_FRAME_BUFFER_ENABLE;
-- 
1.7.12

[PATCH v4 02/12] video: da8xx-fb: fix 24bpp raster configuration

[Afzal Mohammed]

From: "Manjunathappa, Prakash" <prakash.pm@ti.com>

Set only LCD_V2_TFT_24BPP_MODE bit for 24bpp and LCD_V2_TFT_24BPP_UNPACK
bit along with LCD_V2_TFT_24BPP_MODE for 32bpp configuration.

Patch is tested on am335x-evm for 24bpp and da850-evm for 16bpp
configurations.

Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 drivers/video/da8xx-fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 35a33ca..7f92f37 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -550,10 +550,10 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
 	case 4:
 	case 16:
 		break;
-	case 24:
-		reg |= LCD_V2_TFT_24BPP_MODE;
 	case 32:
 		reg |= LCD_V2_TFT_24BPP_UNPACK;
+	case 24:
+		reg |= LCD_V2_TFT_24BPP_MODE;
 		break;
 
 	case 8:
-- 
1.7.12

[PATCH v4 00/12] video: da8xx-fb: am335x DT support

[Afzal Mohammed]

Hi,

This series adds DT support to da8xx-fb driver (device found on
DaVinci and AM335x SoC's). It does certain cleanup's in the process.

This series as compared to previous version uses new registration
interface for clock divider that has constraints on minimum divider
value.

This makes use of Steffen Trumtrar's v16 of display timing DT support.

Testing has been done on AM335x SoC based boards like AM335x EVM. It
has also been verified that display on DA850 EVM (non-DT boot) works
as earlier.

This series is based on v3.8-rc3,
 and is dependent on,
1. Series v16 "of: add display helper" by,
        Steffen Trumtrar <s.trumtrar@pengutronix.de>
2. Patch "da8xx: Allow use by am33xx based devices" by,
        Pantelis Antoniou <panto@antoniou-consulting.com>
3. Series v3 "video: da8xx-fb: runtime timing configuration" by,
        me (Afzal Mohammed <afzal@ti.com>)

To test this series on AM335x based boards,
1. Series v2 "ARM: dts: AM33XX: lcdc support" by,
        me (Afzal Mohammed <afzal@ti.com>),
2. Series "HWMOD fixes for AM33xx PWM submodules and device tree nodes" by,
        Philip, Avinash <avinashphilip@ti.com>
3. Series v2 "clk: divider: prepare for minimum divider" by,
        me (Afzal Mohammed <afzal@ti.com>),
4. Series v2 "ARM: AM335x: LCDC platform support" by,
        me (Afzal Mohammed <afzal@ti.com>),
would be needed.

All above dependencies along with those required for testing is available
@ git://gitorious.org/x0148406-public/linux-kernel.git tags/da8xx-fb-dt-v4

Regards
Afzal

v4: use new registration for clock divider having minimum divider
    requirement and have ifdef'ery in a better way
v3: model CCF clock divider with parent propogation if CCF selected
v2: 2 new patches - one to configure clock rate properly (12/12)and
    other to make io operations safe (1/12)


Afzal Mohammed (11):
  video: da8xx-fb: make io operations safe
  video: da8xx-fb: enable sync lost intr for v2 ip
  video: da8xx-fb: use devres
  video: da8xx-fb: ensure non-null cfg in pdata
  video: da8xx-fb: reorganize panel detection
  video: da8xx-fb: minimal dt support
  video: da8xx-fb: invoke platform callback safely
  video: da8xx-fb: obtain fb_videomode info from dt
  video: da8xx-fb: ensure pdata only for non-dt
  video: da8xx-fb: setup struct lcd_ctrl_config for dt
  video: da8xx-fb: CCF clock divider handling

Manjunathappa, Prakash (1):
  video: da8xx-fb: fix 24bpp raster configuration

 .../devicetree/bindings/video/fb-da8xx.txt         |  37 ++++
 drivers/video/da8xx-fb.c                           | 222 ++++++++++++++++-----
 2 files changed, 206 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt

-- 
1.7.12

[PATCH v4 01/12] video: da8xx-fb: make io operations safe

[Afzal Mohammed]

Replace __raw_readl/__raw_writel with readl/writel; this driver is
reused on ARMv7 (AM335x SoC).

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---

v2: new patch

 drivers/video/da8xx-fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 720604c..35a33ca 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -141,12 +141,12 @@ static int frame_done_flag;
 
 static inline unsigned int lcdc_read(unsigned int addr)
 {
-	return (unsigned int)__raw_readl(da8xx_fb_reg_base + (addr));
+	return (unsigned int)readl(da8xx_fb_reg_base + (addr));
 }
 
 static inline void lcdc_write(unsigned int val, unsigned int addr)
 {
-	__raw_writel(val, da8xx_fb_reg_base + (addr));
+	writel(val, da8xx_fb_reg_base + (addr));
 }
 
 struct da8xx_fb_par {
-- 
1.7.12

Re: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mike Turquette]

Quoting Afzal Mohammed (2013-01-23 03:48:56)
<snip>
> +static inline void da8xx_fb_clkc_enable(void)
> +{
>         if (lcd_revision == LCD_VERSION_2)
>                 lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
>                                 LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
>  }
>  
> -static inline void da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> +#ifdef CONFIG_COMMON_CLK
> +static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> +                                                   struct fb_videomode *mode)
> +{
> +       int ret;
> +
> +       ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
> +       if (IS_ERR_VALUE(ret)) {
> +               dev_err(par->dev, "unable to setup pixel clock of %u ps",
> +                       mode->pixclock);
> +               return ret;
> +       }
> +       da8xx_fb_clkc_enable();

It looks like you are using the legacy method to enable/disable the
clock and using the CCF basic divider to set the rate.  This feels a bit
hacky to me.  If you want to model your clock in CCF then you should
probably model the whole clock, not just the rate-change aspects of it.

Have you looked at the composite clock patches from Prashant?  Those
might give you the divider+gate properties that you are looking for:
http://article.gmane.org/gmane.linux.kernel/1416697

Regards,
Mike

Re: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mike Turquette]

Quoting Afzal Mohammed (2013-01-23 03:48:56)
<snip>
> +static inline void da8xx_fb_clkc_enable(void)
> +{
>         if (lcd_revision == LCD_VERSION_2)
>                 lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
>                                 LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
>  }
>  
> -static inline void da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> +#ifdef CONFIG_COMMON_CLK
> +static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> +                                                   struct fb_videomode *mode)
> +{
> +       int ret;
> +
> +       ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
> +       if (IS_ERR_VALUE(ret)) {
> +               dev_err(par->dev, "unable to setup pixel clock of %u ps",
> +                       mode->pixclock);
> +               return ret;
> +       }
> +       da8xx_fb_clkc_enable();

It looks like you are using the legacy method to enable/disable the
clock and using the CCF basic divider to set the rate.  This feels a bit
hacky to me.  If you want to model your clock in CCF then you should
probably model the whole clock, not just the rate-change aspects of it.

Have you looked at the composite clock patches from Prashant?  Those
might give you the divider+gate properties that you are looking for:
http://article.gmane.org/gmane.linux.kernel/1416697

Regards,
Mike

Re: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mike Turquette]

Quoting Afzal Mohammed (2013-01-23 03:48:56)
<snip>
> +static inline void da8xx_fb_clkc_enable(void)
> +{
>         if (lcd_revision = LCD_VERSION_2)
>                 lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
>                                 LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
>  }
>  
> -static inline void da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> +#ifdef CONFIG_COMMON_CLK
> +static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> +                                                   struct fb_videomode *mode)
> +{
> +       int ret;
> +
> +       ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
> +       if (IS_ERR_VALUE(ret)) {
> +               dev_err(par->dev, "unable to setup pixel clock of %u ps",
> +                       mode->pixclock);
> +               return ret;
> +       }
> +       da8xx_fb_clkc_enable();

It looks like you are using the legacy method to enable/disable the
clock and using the CCF basic divider to set the rate.  This feels a bit
hacky to me.  If you want to model your clock in CCF then you should
probably model the whole clock, not just the rate-change aspects of it.

Have you looked at the composite clock patches from Prashant?  Those
might give you the divider+gate properties that you are looking for:
http://article.gmane.org/gmane.linux.kernel/1416697

Regards,
Mike

RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mohammed, Afzal]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2818 bytes --]

Hi Mike,

On Thu, Jan 24, 2013 at 01:52:04, Mike Turquette wrote:
> Quoting Afzal Mohammed (2013-01-23 03:48:56)

> > +static inline void da8xx_fb_clkc_enable(void)
> > +{
> >         if (lcd_revision == LCD_VERSION_2)
> >                 lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
> >                                 LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);

> > +static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> > +                                                   struct fb_videomode *mode)
> > +{

> > +       ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
> > +       if (IS_ERR_VALUE(ret)) {
> > +               dev_err(par->dev, "unable to setup pixel clock of %u ps",
> > +                       mode->pixclock);
> > +               return ret;
> > +       }
> > +       da8xx_fb_clkc_enable();

> It looks like you are using the legacy method to enable/disable the
> clock and using the CCF basic divider to set the rate.  This feels a bit
> hacky to me.  If you want to model your clock in CCF then you should
> probably model the whole clock, not just the rate-change aspects of it.

Initially I thought about it, but seeing requirement of 3 gate clocks
(due to 3 bits meant for different purposes - DMA, LIDD and CORE
functionalities), felt that having 4 clocks (3 gate + 1 divider) in
driver would be an overdesign [leaving a branch instead of a leaf of
the tree in driver ;)].

> Have you looked at the composite clock patches from Prashant?  Those
> might give you the divider+gate properties that you are looking for:
> http://article.gmane.org/gmane.linux.kernel/1416697

Thanks for the pointer,

Now with the composite clock in mind, it was tried to relate to what
was required for the present scenario.

So there are 3 - LIDD is actually not for present use case, CORE could
be clubbed with the divider to have a composite clock. And CORE is
in functional clock path and logically it's perfectly alright to have
the composite clock.

And now we are left with DMA, this is actually in the interface clock
path which driver in unaware. An option would be to have DMA clock
as child of CORE plus divider composite clock, even though logically
DMA can't be considered in the same path.

Also tried not enabling DMA clock, but driver is able to provide
display without any issues, so was thinking whether to avoid
instantiating DMA clock at all and hence to have a simple single
composite clock. Trying to get information internally on whether
not setting DMA clock bits would actually make a difference.

If you have any opinion on how to deal here, let me know.

Regards
Afzal


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mohammed, Afzal]

Hi Mike,

On Thu, Jan 24, 2013 at 01:52:04, Mike Turquette wrote:
> Quoting Afzal Mohammed (2013-01-23 03:48:56)

> > +static inline void da8xx_fb_clkc_enable(void)
> > +{
> >         if (lcd_revision == LCD_VERSION_2)
> >                 lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
> >                                 LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);

> > +static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> > +                                                   struct fb_videomode *mode)
> > +{

> > +       ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
> > +       if (IS_ERR_VALUE(ret)) {
> > +               dev_err(par->dev, "unable to setup pixel clock of %u ps",
> > +                       mode->pixclock);
> > +               return ret;
> > +       }
> > +       da8xx_fb_clkc_enable();

> It looks like you are using the legacy method to enable/disable the
> clock and using the CCF basic divider to set the rate.  This feels a bit
> hacky to me.  If you want to model your clock in CCF then you should
> probably model the whole clock, not just the rate-change aspects of it.

Initially I thought about it, but seeing requirement of 3 gate clocks
(due to 3 bits meant for different purposes - DMA, LIDD and CORE
functionalities), felt that having 4 clocks (3 gate + 1 divider) in
driver would be an overdesign [leaving a branch instead of a leaf of
the tree in driver ;)].

> Have you looked at the composite clock patches from Prashant?  Those
> might give you the divider+gate properties that you are looking for:
> http://article.gmane.org/gmane.linux.kernel/1416697

Thanks for the pointer,

Now with the composite clock in mind, it was tried to relate to what
was required for the present scenario.

So there are 3 - LIDD is actually not for present use case, CORE could
be clubbed with the divider to have a composite clock. And CORE is
in functional clock path and logically it's perfectly alright to have
the composite clock.

And now we are left with DMA, this is actually in the interface clock
path which driver in unaware. An option would be to have DMA clock
as child of CORE plus divider composite clock, even though logically
DMA can't be considered in the same path.

Also tried not enabling DMA clock, but driver is able to provide
display without any issues, so was thinking whether to avoid
instantiating DMA clock at all and hence to have a simple single
composite clock. Trying to get information internally on whether
not setting DMA clock bits would actually make a difference.

If you have any opinion on how to deal here, let me know.

Regards
Afzal

RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mohammed, Afzal]

SGkgTWlrZSwNCg0KT24gVGh1LCBKYW4gMjQsIDIwMTMgYXQgMDE6NTI6MDQsIE1pa2UgVHVycXVl
dHRlIHdyb3RlOg0KPiBRdW90aW5nIEFmemFsIE1vaGFtbWVkICgyMDEzLTAxLTIzIDAzOjQ4OjU2
KQ0KDQo+ID4gK3N0YXRpYyBpbmxpbmUgdm9pZCBkYTh4eF9mYl9jbGtjX2VuYWJsZSh2b2lkKQ0K
PiA+ICt7DQo+ID4gICAgICAgICBpZiAobGNkX3JldmlzaW9uID09IExDRF9WRVJTSU9OXzIpDQo+
ID4gICAgICAgICAgICAgICAgIGxjZGNfd3JpdGUoTENEX1YyX0RNQV9DTEtfRU4gfCBMQ0RfVjJf
TElERF9DTEtfRU4gfA0KPiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgTENEX1Yy
X0NPUkVfQ0xLX0VOLCBMQ0RfQ0xLX0VOQUJMRV9SRUcpOw0KDQo+ID4gK3N0YXRpYyBpbmxpbmUg
aW50IGRhOHh4X2ZiX2NhbGNfY29uZmlnX2Nsa19kaXZpZGVyKHN0cnVjdCBkYTh4eF9mYl9wYXIg
KnBhciwNCj4gPiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgc3RydWN0IGZiX3ZpZGVvbW9kZSAqbW9kZSkNCj4gPiArew0KDQo+ID4gKyAgICAgICBy
ZXQgPSBjbGtfc2V0X3JhdGUocGFyLT5jaGlsZF9jbGssIFBJQ09TMktIWihtb2RlLT5waXhjbG9j
aykgKiAxMDAwKTsNCj4gPiArICAgICAgIGlmIChJU19FUlJfVkFMVUUocmV0KSkgew0KPiA+ICsg
ICAgICAgICAgICAgICBkZXZfZXJyKHBhci0+ZGV2LCAidW5hYmxlIHRvIHNldHVwIHBpeGVsIGNs
b2NrIG9mICV1IHBzIiwNCj4gPiArICAgICAgICAgICAgICAgICAgICAgICBtb2RlLT5waXhjbG9j
ayk7DQo+ID4gKyAgICAgICAgICAgICAgIHJldHVybiByZXQ7DQo+ID4gKyAgICAgICB9DQo+ID4g
KyAgICAgICBkYTh4eF9mYl9jbGtjX2VuYWJsZSgpOw0KDQo+IEl0IGxvb2tzIGxpa2UgeW91IGFy
ZSB1c2luZyB0aGUgbGVnYWN5IG1ldGhvZCB0byBlbmFibGUvZGlzYWJsZSB0aGUNCj4gY2xvY2sg
YW5kIHVzaW5nIHRoZSBDQ0YgYmFzaWMgZGl2aWRlciB0byBzZXQgdGhlIHJhdGUuICBUaGlzIGZl
ZWxzIGEgYml0DQo+IGhhY2t5IHRvIG1lLiAgSWYgeW91IHdhbnQgdG8gbW9kZWwgeW91ciBjbG9j
ayBpbiBDQ0YgdGhlbiB5b3Ugc2hvdWxkDQo+IHByb2JhYmx5IG1vZGVsIHRoZSB3aG9sZSBjbG9j
aywgbm90IGp1c3QgdGhlIHJhdGUtY2hhbmdlIGFzcGVjdHMgb2YgaXQuDQoNCkluaXRpYWxseSBJ
IHRob3VnaHQgYWJvdXQgaXQsIGJ1dCBzZWVpbmcgcmVxdWlyZW1lbnQgb2YgMyBnYXRlIGNsb2Nr
cw0KKGR1ZSB0byAzIGJpdHMgbWVhbnQgZm9yIGRpZmZlcmVudCBwdXJwb3NlcyAtIERNQSwgTElE
RCBhbmQgQ09SRQ0KZnVuY3Rpb25hbGl0aWVzKSwgZmVsdCB0aGF0IGhhdmluZyA0IGNsb2NrcyAo
MyBnYXRlICsgMSBkaXZpZGVyKSBpbg0KZHJpdmVyIHdvdWxkIGJlIGFuIG92ZXJkZXNpZ24gW2xl
YXZpbmcgYSBicmFuY2ggaW5zdGVhZCBvZiBhIGxlYWYgb2YNCnRoZSB0cmVlIGluIGRyaXZlciA7
KV0uDQoNCj4gSGF2ZSB5b3UgbG9va2VkIGF0IHRoZSBjb21wb3NpdGUgY2xvY2sgcGF0Y2hlcyBm
cm9tIFByYXNoYW50PyAgVGhvc2UNCj4gbWlnaHQgZ2l2ZSB5b3UgdGhlIGRpdmlkZXIrZ2F0ZSBw
cm9wZXJ0aWVzIHRoYXQgeW91IGFyZSBsb29raW5nIGZvcjoNCj4gaHR0cDovL2FydGljbGUuZ21h
bmUub3JnL2dtYW5lLmxpbnV4Lmtlcm5lbC8xNDE2Njk3DQoNClRoYW5rcyBmb3IgdGhlIHBvaW50
ZXIsDQoNCk5vdyB3aXRoIHRoZSBjb21wb3NpdGUgY2xvY2sgaW4gbWluZCwgaXQgd2FzIHRyaWVk
IHRvIHJlbGF0ZSB0byB3aGF0DQp3YXMgcmVxdWlyZWQgZm9yIHRoZSBwcmVzZW50IHNjZW5hcmlv
Lg0KDQpTbyB0aGVyZSBhcmUgMyAtIExJREQgaXMgYWN0dWFsbHkgbm90IGZvciBwcmVzZW50IHVz
ZSBjYXNlLCBDT1JFIGNvdWxkDQpiZSBjbHViYmVkIHdpdGggdGhlIGRpdmlkZXIgdG8gaGF2ZSBh
IGNvbXBvc2l0ZSBjbG9jay4gQW5kIENPUkUgaXMNCmluIGZ1bmN0aW9uYWwgY2xvY2sgcGF0aCBh
bmQgbG9naWNhbGx5IGl0J3MgcGVyZmVjdGx5IGFscmlnaHQgdG8gaGF2ZQ0KdGhlIGNvbXBvc2l0
ZSBjbG9jay4NCg0KQW5kIG5vdyB3ZSBhcmUgbGVmdCB3aXRoIERNQSwgdGhpcyBpcyBhY3R1YWxs
eSBpbiB0aGUgaW50ZXJmYWNlIGNsb2NrDQpwYXRoIHdoaWNoIGRyaXZlciBpbiB1bmF3YXJlLiBB
biBvcHRpb24gd291bGQgYmUgdG8gaGF2ZSBETUEgY2xvY2sNCmFzIGNoaWxkIG9mIENPUkUgcGx1
cyBkaXZpZGVyIGNvbXBvc2l0ZSBjbG9jaywgZXZlbiB0aG91Z2ggbG9naWNhbGx5DQpETUEgY2Fu
J3QgYmUgY29uc2lkZXJlZCBpbiB0aGUgc2FtZSBwYXRoLg0KDQpBbHNvIHRyaWVkIG5vdCBlbmFi
bGluZyBETUEgY2xvY2ssIGJ1dCBkcml2ZXIgaXMgYWJsZSB0byBwcm92aWRlDQpkaXNwbGF5IHdp
dGhvdXQgYW55IGlzc3Vlcywgc28gd2FzIHRoaW5raW5nIHdoZXRoZXIgdG8gYXZvaWQNCmluc3Rh
bnRpYXRpbmcgRE1BIGNsb2NrIGF0IGFsbCBhbmQgaGVuY2UgdG8gaGF2ZSBhIHNpbXBsZSBzaW5n
bGUNCmNvbXBvc2l0ZSBjbG9jay4gVHJ5aW5nIHRvIGdldCBpbmZvcm1hdGlvbiBpbnRlcm5hbGx5
IG9uIHdoZXRoZXINCm5vdCBzZXR0aW5nIERNQSBjbG9jayBiaXRzIHdvdWxkIGFjdHVhbGx5IG1h
a2UgYSBkaWZmZXJlbmNlLg0KDQpJZiB5b3UgaGF2ZSBhbnkgb3BpbmlvbiBvbiBob3cgdG8gZGVh
bCBoZXJlLCBsZXQgbWUga25vdy4NCg0KUmVnYXJkcw0KQWZ6YWwNCg0KDQo

Re: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mike Turquette]

Quoting Mohammed, Afzal (2013-01-24 03:36:02)
> Hi Mike,
> 
> On Thu, Jan 24, 2013 at 01:52:04, Mike Turquette wrote:
> > Quoting Afzal Mohammed (2013-01-23 03:48:56)
> 
> > > +static inline void da8xx_fb_clkc_enable(void)
> > > +{
> > >         if (lcd_revision == LCD_VERSION_2)
> > >                 lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
> > >                                 LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
> 
> > > +static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> > > +                                                   struct fb_videomode *mode)
> > > +{
> 
> > > +       ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
> > > +       if (IS_ERR_VALUE(ret)) {
> > > +               dev_err(par->dev, "unable to setup pixel clock of %u ps",
> > > +                       mode->pixclock);
> > > +               return ret;
> > > +       }
> > > +       da8xx_fb_clkc_enable();
> 
> > It looks like you are using the legacy method to enable/disable the
> > clock and using the CCF basic divider to set the rate.  This feels a bit
> > hacky to me.  If you want to model your clock in CCF then you should
> > probably model the whole clock, not just the rate-change aspects of it.
> 
> Initially I thought about it, but seeing requirement of 3 gate clocks
> (due to 3 bits meant for different purposes - DMA, LIDD and CORE
> functionalities), felt that having 4 clocks (3 gate + 1 divider) in
> driver would be an overdesign [leaving a branch instead of a leaf of
> the tree in driver ;)].
> 
> > Have you looked at the composite clock patches from Prashant?  Those
> > might give you the divider+gate properties that you are looking for:
> > http://article.gmane.org/gmane.linux.kernel/1416697
> 
> Thanks for the pointer,
> 
> Now with the composite clock in mind, it was tried to relate to what
> was required for the present scenario.
> 
> So there are 3 - LIDD is actually not for present use case, CORE could
> be clubbed with the divider to have a composite clock. And CORE is
> in functional clock path and logically it's perfectly alright to have
> the composite clock.
> 

Some of the clock names are a bit generic, so a question that I'm going
to repeat throughout my response: "is this clock only inside of your
video IP ?"

Regarding the CORE clock, is this only inside of your IP or are you
referring to the SoC CORE clock which is driven by a DPLL and clocks
DDR and many other peripherals (often MMC, UART, etc)?

Note that this is from my past experience with OMAP, and I'm making an
assumption that the clock scheme between OMAP and Da Vinci/AM335x parts
isn't very different.

Is there a public TRM I can look at?  It would help me understand this
without having to ask you so many annoying questions ;)

> And now we are left with DMA, this is actually in the interface clock
> path which driver in unaware. An option would be to have DMA clock
> as child of CORE plus divider composite clock, even though logically
> DMA can't be considered in the same path.
> 

Why is the driver unaware of the interface clk?  For instance OMAP3 had
separate fclk and iclk for IPs and drivers would call clk_enable on
both.  Or am I misunderstanding something?

In general I don't think the clock subtree should be modeled in a way
that is convenient for software, but instead model the actual hardware.
Trust me, if you don't model the actual hardware then you will be very
confused when you come back and revisit this code in 6 months and can't
remember why things are so weird looking.

Thanks,
Mike

> Also tried not enabling DMA clock, but driver is able to provide
> display without any issues, so was thinking whether to avoid
> instantiating DMA clock at all and hence to have a simple single
> composite clock. Trying to get information internally on whether
> not setting DMA clock bits would actually make a difference.
> 
> If you have any opinion on how to deal here, let me know.
> 
> Regards
> Afzal

Re: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mike Turquette]

Quoting Mohammed, Afzal (2013-01-24 03:36:02)
> Hi Mike,
> 
> On Thu, Jan 24, 2013 at 01:52:04, Mike Turquette wrote:
> > Quoting Afzal Mohammed (2013-01-23 03:48:56)
> 
> > > +static inline void da8xx_fb_clkc_enable(void)
> > > +{
> > >         if (lcd_revision == LCD_VERSION_2)
> > >                 lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
> > >                                 LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
> 
> > > +static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> > > +                                                   struct fb_videomode *mode)
> > > +{
> 
> > > +       ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
> > > +       if (IS_ERR_VALUE(ret)) {
> > > +               dev_err(par->dev, "unable to setup pixel clock of %u ps",
> > > +                       mode->pixclock);
> > > +               return ret;
> > > +       }
> > > +       da8xx_fb_clkc_enable();
> 
> > It looks like you are using the legacy method to enable/disable the
> > clock and using the CCF basic divider to set the rate.  This feels a bit
> > hacky to me.  If you want to model your clock in CCF then you should
> > probably model the whole clock, not just the rate-change aspects of it.
> 
> Initially I thought about it, but seeing requirement of 3 gate clocks
> (due to 3 bits meant for different purposes - DMA, LIDD and CORE
> functionalities), felt that having 4 clocks (3 gate + 1 divider) in
> driver would be an overdesign [leaving a branch instead of a leaf of
> the tree in driver ;)].
> 
> > Have you looked at the composite clock patches from Prashant?  Those
> > might give you the divider+gate properties that you are looking for:
> > http://article.gmane.org/gmane.linux.kernel/1416697
> 
> Thanks for the pointer,
> 
> Now with the composite clock in mind, it was tried to relate to what
> was required for the present scenario.
> 
> So there are 3 - LIDD is actually not for present use case, CORE could
> be clubbed with the divider to have a composite clock. And CORE is
> in functional clock path and logically it's perfectly alright to have
> the composite clock.
> 

Some of the clock names are a bit generic, so a question that I'm going
to repeat throughout my response: "is this clock only inside of your
video IP ?"

Regarding the CORE clock, is this only inside of your IP or are you
referring to the SoC CORE clock which is driven by a DPLL and clocks
DDR and many other peripherals (often MMC, UART, etc)?

Note that this is from my past experience with OMAP, and I'm making an
assumption that the clock scheme between OMAP and Da Vinci/AM335x parts
isn't very different.

Is there a public TRM I can look at?  It would help me understand this
without having to ask you so many annoying questions ;)

> And now we are left with DMA, this is actually in the interface clock
> path which driver in unaware. An option would be to have DMA clock
> as child of CORE plus divider composite clock, even though logically
> DMA can't be considered in the same path.
> 

Why is the driver unaware of the interface clk?  For instance OMAP3 had
separate fclk and iclk for IPs and drivers would call clk_enable on
both.  Or am I misunderstanding something?

In general I don't think the clock subtree should be modeled in a way
that is convenient for software, but instead model the actual hardware.
Trust me, if you don't model the actual hardware then you will be very
confused when you come back and revisit this code in 6 months and can't
remember why things are so weird looking.

Thanks,
Mike

> Also tried not enabling DMA clock, but driver is able to provide
> display without any issues, so was thinking whether to avoid
> instantiating DMA clock at all and hence to have a simple single
> composite clock. Trying to get information internally on whether
> not setting DMA clock bits would actually make a difference.
> 
> If you have any opinion on how to deal here, let me know.
> 
> Regards
> Afzal

Re: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mike Turquette]

Quoting Mohammed, Afzal (2013-01-24 03:36:02)
> Hi Mike,
> 
> On Thu, Jan 24, 2013 at 01:52:04, Mike Turquette wrote:
> > Quoting Afzal Mohammed (2013-01-23 03:48:56)
> 
> > > +static inline void da8xx_fb_clkc_enable(void)
> > > +{
> > >         if (lcd_revision = LCD_VERSION_2)
> > >                 lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
> > >                                 LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
> 
> > > +static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> > > +                                                   struct fb_videomode *mode)
> > > +{
> 
> > > +       ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
> > > +       if (IS_ERR_VALUE(ret)) {
> > > +               dev_err(par->dev, "unable to setup pixel clock of %u ps",
> > > +                       mode->pixclock);
> > > +               return ret;
> > > +       }
> > > +       da8xx_fb_clkc_enable();
> 
> > It looks like you are using the legacy method to enable/disable the
> > clock and using the CCF basic divider to set the rate.  This feels a bit
> > hacky to me.  If you want to model your clock in CCF then you should
> > probably model the whole clock, not just the rate-change aspects of it.
> 
> Initially I thought about it, but seeing requirement of 3 gate clocks
> (due to 3 bits meant for different purposes - DMA, LIDD and CORE
> functionalities), felt that having 4 clocks (3 gate + 1 divider) in
> driver would be an overdesign [leaving a branch instead of a leaf of
> the tree in driver ;)].
> 
> > Have you looked at the composite clock patches from Prashant?  Those
> > might give you the divider+gate properties that you are looking for:
> > http://article.gmane.org/gmane.linux.kernel/1416697
> 
> Thanks for the pointer,
> 
> Now with the composite clock in mind, it was tried to relate to what
> was required for the present scenario.
> 
> So there are 3 - LIDD is actually not for present use case, CORE could
> be clubbed with the divider to have a composite clock. And CORE is
> in functional clock path and logically it's perfectly alright to have
> the composite clock.
> 

Some of the clock names are a bit generic, so a question that I'm going
to repeat throughout my response: "is this clock only inside of your
video IP ?"

Regarding the CORE clock, is this only inside of your IP or are you
referring to the SoC CORE clock which is driven by a DPLL and clocks
DDR and many other peripherals (often MMC, UART, etc)?

Note that this is from my past experience with OMAP, and I'm making an
assumption that the clock scheme between OMAP and Da Vinci/AM335x parts
isn't very different.

Is there a public TRM I can look at?  It would help me understand this
without having to ask you so many annoying questions ;)

> And now we are left with DMA, this is actually in the interface clock
> path which driver in unaware. An option would be to have DMA clock
> as child of CORE plus divider composite clock, even though logically
> DMA can't be considered in the same path.
> 

Why is the driver unaware of the interface clk?  For instance OMAP3 had
separate fclk and iclk for IPs and drivers would call clk_enable on
both.  Or am I misunderstanding something?

In general I don't think the clock subtree should be modeled in a way
that is convenient for software, but instead model the actual hardware.
Trust me, if you don't model the actual hardware then you will be very
confused when you come back and revisit this code in 6 months and can't
remember why things are so weird looking.

Thanks,
Mike

> Also tried not enabling DMA clock, but driver is able to provide
> display without any issues, so was thinking whether to avoid
> instantiating DMA clock at all and hence to have a simple single
> composite clock. Trying to get information internally on whether
> not setting DMA clock bits would actually make a difference.
> 
> If you have any opinion on how to deal here, let me know.
> 
> Regards
> Afzal

RE: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mohammed, Afzal]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2970 bytes --]

Hi Mike,

On Thu, Jan 24, 2013 at 22:30:44, Mike Turquette wrote:
> Quoting Mohammed, Afzal (2013-01-24 03:36:02)

> > So there are 3 - LIDD is actually not for present use case, CORE could
> > be clubbed with the divider to have a composite clock. And CORE is
> > in functional clock path and logically it's perfectly alright to have
> > the composite clock.

> Some of the clock names are a bit generic, so a question that I'm going
> to repeat throughout my response: "is this clock only inside of your
> video IP ?"

Yes these three clocks are inside LCDC IP.

> Regarding the CORE clock, is this only inside of your IP or are you
> referring to the SoC CORE clock which is driven by a DPLL and clocks
> DDR and many other peripherals (often MMC, UART, etc)?

Sorry for the confusion, here CORE refers to clock inside LCDC IP. This
CORE should not be confused with CORE PLL. Actually I used CORE so that
it corresponds to the nomenclature in LCDC section of TRM.

> Note that this is from my past experience with OMAP, and I'm making an
> assumption that the clock scheme between OMAP and Da Vinci/AM335x parts
> isn't very different.

Additional detail: DaVinci doesn't have these 3 clocks controls available,
so these three are required only on AM335x (which has IP version 2 )

> Is there a public TRM I can look at?  It would help me understand this
> without having to ask you so many annoying questions ;)

No problem, http://www.ti.com/product/am3359


> > And now we are left with DMA, this is actually in the interface clock
> > path which driver in unaware. An option would be to have DMA clock
> > as child of CORE plus divider composite clock, even though logically
> > DMA can't be considered in the same path.

> Why is the driver unaware of the interface clk?  For instance OMAP3 had
> separate fclk and iclk for IPs and drivers would call clk_enable on
> both.  Or am I misunderstanding something?

HWMOD handles enabling those upon pm_runtime calls, HWMOD makes an alias
for main clock with "fck", but not for "ick", so currently "ick" is
unavailable for the driver, continued below ..

> In general I don't think the clock subtree should be modeled in a way
> that is convenient for software, but instead model the actual hardware.
> Trust me, if you don't model the actual hardware then you will be very
> confused when you come back and revisit this code in 6 months and can't
> remember why things are so weird looking.

Ok, then it seems an omap clock entry for con-id "ick" should be created
as follows (dpll_core_m4_ck supplies interface clock),

CLK("4830e000.lcdc",    "ick",          &dpll_core_m4_ck,       CK_AM33XX)

And then in the driver, DMA gate clock should be made a child of this clock
(obtained with con-id "ick").

Let me know your opinion on this.

Regards
Afzal



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mohammed, Afzal]

Hi Mike,

On Thu, Jan 24, 2013 at 22:30:44, Mike Turquette wrote:
> Quoting Mohammed, Afzal (2013-01-24 03:36:02)

> > So there are 3 - LIDD is actually not for present use case, CORE could
> > be clubbed with the divider to have a composite clock. And CORE is
> > in functional clock path and logically it's perfectly alright to have
> > the composite clock.

> Some of the clock names are a bit generic, so a question that I'm going
> to repeat throughout my response: "is this clock only inside of your
> video IP ?"

Yes these three clocks are inside LCDC IP.

> Regarding the CORE clock, is this only inside of your IP or are you
> referring to the SoC CORE clock which is driven by a DPLL and clocks
> DDR and many other peripherals (often MMC, UART, etc)?

Sorry for the confusion, here CORE refers to clock inside LCDC IP. This
CORE should not be confused with CORE PLL. Actually I used CORE so that
it corresponds to the nomenclature in LCDC section of TRM.

> Note that this is from my past experience with OMAP, and I'm making an
> assumption that the clock scheme between OMAP and Da Vinci/AM335x parts
> isn't very different.

Additional detail: DaVinci doesn't have these 3 clocks controls available,
so these three are required only on AM335x (which has IP version 2 )

> Is there a public TRM I can look at?  It would help me understand this
> without having to ask you so many annoying questions ;)

No problem, http://www.ti.com/product/am3359


> > And now we are left with DMA, this is actually in the interface clock
> > path which driver in unaware. An option would be to have DMA clock
> > as child of CORE plus divider composite clock, even though logically
> > DMA can't be considered in the same path.

> Why is the driver unaware of the interface clk?  For instance OMAP3 had
> separate fclk and iclk for IPs and drivers would call clk_enable on
> both.  Or am I misunderstanding something?

HWMOD handles enabling those upon pm_runtime calls, HWMOD makes an alias
for main clock with "fck", but not for "ick", so currently "ick" is
unavailable for the driver, continued below ..

> In general I don't think the clock subtree should be modeled in a way
> that is convenient for software, but instead model the actual hardware.
> Trust me, if you don't model the actual hardware then you will be very
> confused when you come back and revisit this code in 6 months and can't
> remember why things are so weird looking.

Ok, then it seems an omap clock entry for con-id "ick" should be created
as follows (dpll_core_m4_ck supplies interface clock),

CLK("4830e000.lcdc",    "ick",          &dpll_core_m4_ck,       CK_AM33XX)

And then in the driver, DMA gate clock should be made a child of this clock
(obtained with con-id "ick").

Let me know your opinion on this.

Regards
Afzal

RE: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mohammed, Afzal]

SGkgTWlrZSwNCg0KT24gVGh1LCBKYW4gMjQsIDIwMTMgYXQgMjI6MzA6NDQsIE1pa2UgVHVycXVl
dHRlIHdyb3RlOg0KPiBRdW90aW5nIE1vaGFtbWVkLCBBZnphbCAoMjAxMy0wMS0yNCAwMzozNjow
MikNCg0KPiA+IFNvIHRoZXJlIGFyZSAzIC0gTElERCBpcyBhY3R1YWxseSBub3QgZm9yIHByZXNl
bnQgdXNlIGNhc2UsIENPUkUgY291bGQNCj4gPiBiZSBjbHViYmVkIHdpdGggdGhlIGRpdmlkZXIg
dG8gaGF2ZSBhIGNvbXBvc2l0ZSBjbG9jay4gQW5kIENPUkUgaXMNCj4gPiBpbiBmdW5jdGlvbmFs
IGNsb2NrIHBhdGggYW5kIGxvZ2ljYWxseSBpdCdzIHBlcmZlY3RseSBhbHJpZ2h0IHRvIGhhdmUN
Cj4gPiB0aGUgY29tcG9zaXRlIGNsb2NrLg0KDQo+IFNvbWUgb2YgdGhlIGNsb2NrIG5hbWVzIGFy
ZSBhIGJpdCBnZW5lcmljLCBzbyBhIHF1ZXN0aW9uIHRoYXQgSSdtIGdvaW5nDQo+IHRvIHJlcGVh
dCB0aHJvdWdob3V0IG15IHJlc3BvbnNlOiAiaXMgdGhpcyBjbG9jayBvbmx5IGluc2lkZSBvZiB5
b3VyDQo+IHZpZGVvIElQID8iDQoNClllcyB0aGVzZSB0aHJlZSBjbG9ja3MgYXJlIGluc2lkZSBM
Q0RDIElQLg0KDQo+IFJlZ2FyZGluZyB0aGUgQ09SRSBjbG9jaywgaXMgdGhpcyBvbmx5IGluc2lk
ZSBvZiB5b3VyIElQIG9yIGFyZSB5b3UNCj4gcmVmZXJyaW5nIHRvIHRoZSBTb0MgQ09SRSBjbG9j
ayB3aGljaCBpcyBkcml2ZW4gYnkgYSBEUExMIGFuZCBjbG9ja3MNCj4gRERSIGFuZCBtYW55IG90
aGVyIHBlcmlwaGVyYWxzIChvZnRlbiBNTUMsIFVBUlQsIGV0Yyk/DQoNClNvcnJ5IGZvciB0aGUg
Y29uZnVzaW9uLCBoZXJlIENPUkUgcmVmZXJzIHRvIGNsb2NrIGluc2lkZSBMQ0RDIElQLiBUaGlz
DQpDT1JFIHNob3VsZCBub3QgYmUgY29uZnVzZWQgd2l0aCBDT1JFIFBMTC4gQWN0dWFsbHkgSSB1
c2VkIENPUkUgc28gdGhhdA0KaXQgY29ycmVzcG9uZHMgdG8gdGhlIG5vbWVuY2xhdHVyZSBpbiBM
Q0RDIHNlY3Rpb24gb2YgVFJNLg0KDQo+IE5vdGUgdGhhdCB0aGlzIGlzIGZyb20gbXkgcGFzdCBl
eHBlcmllbmNlIHdpdGggT01BUCwgYW5kIEknbSBtYWtpbmcgYW4NCj4gYXNzdW1wdGlvbiB0aGF0
IHRoZSBjbG9jayBzY2hlbWUgYmV0d2VlbiBPTUFQIGFuZCBEYSBWaW5jaS9BTTMzNXggcGFydHMN
Cj4gaXNuJ3QgdmVyeSBkaWZmZXJlbnQuDQoNCkFkZGl0aW9uYWwgZGV0YWlsOiBEYVZpbmNpIGRv
ZXNuJ3QgaGF2ZSB0aGVzZSAzIGNsb2NrcyBjb250cm9scyBhdmFpbGFibGUsDQpzbyB0aGVzZSB0
aHJlZSBhcmUgcmVxdWlyZWQgb25seSBvbiBBTTMzNXggKHdoaWNoIGhhcyBJUCB2ZXJzaW9uIDIg
KQ0KDQo+IElzIHRoZXJlIGEgcHVibGljIFRSTSBJIGNhbiBsb29rIGF0PyAgSXQgd291bGQgaGVs
cCBtZSB1bmRlcnN0YW5kIHRoaXMNCj4gd2l0aG91dCBoYXZpbmcgdG8gYXNrIHlvdSBzbyBtYW55
IGFubm95aW5nIHF1ZXN0aW9ucyA7KQ0KDQpObyBwcm9ibGVtLCBodHRwOi8vd3d3LnRpLmNvbS9w
cm9kdWN0L2FtMzM1OQ0KDQoNCj4gPiBBbmQgbm93IHdlIGFyZSBsZWZ0IHdpdGggRE1BLCB0aGlz
IGlzIGFjdHVhbGx5IGluIHRoZSBpbnRlcmZhY2UgY2xvY2sNCj4gPiBwYXRoIHdoaWNoIGRyaXZl
ciBpbiB1bmF3YXJlLiBBbiBvcHRpb24gd291bGQgYmUgdG8gaGF2ZSBETUEgY2xvY2sNCj4gPiBh
cyBjaGlsZCBvZiBDT1JFIHBsdXMgZGl2aWRlciBjb21wb3NpdGUgY2xvY2ssIGV2ZW4gdGhvdWdo
IGxvZ2ljYWxseQ0KPiA+IERNQSBjYW4ndCBiZSBjb25zaWRlcmVkIGluIHRoZSBzYW1lIHBhdGgu
DQoNCj4gV2h5IGlzIHRoZSBkcml2ZXIgdW5hd2FyZSBvZiB0aGUgaW50ZXJmYWNlIGNsaz8gIEZv
ciBpbnN0YW5jZSBPTUFQMyBoYWQNCj4gc2VwYXJhdGUgZmNsayBhbmQgaWNsayBmb3IgSVBzIGFu
ZCBkcml2ZXJzIHdvdWxkIGNhbGwgY2xrX2VuYWJsZSBvbg0KPiBib3RoLiAgT3IgYW0gSSBtaXN1
bmRlcnN0YW5kaW5nIHNvbWV0aGluZz8NCg0KSFdNT0QgaGFuZGxlcyBlbmFibGluZyB0aG9zZSB1
cG9uIHBtX3J1bnRpbWUgY2FsbHMsIEhXTU9EIG1ha2VzIGFuIGFsaWFzDQpmb3IgbWFpbiBjbG9j
ayB3aXRoICJmY2siLCBidXQgbm90IGZvciAiaWNrIiwgc28gY3VycmVudGx5ICJpY2siIGlzDQp1
bmF2YWlsYWJsZSBmb3IgdGhlIGRyaXZlciwgY29udGludWVkIGJlbG93IC4uDQoNCj4gSW4gZ2Vu
ZXJhbCBJIGRvbid0IHRoaW5rIHRoZSBjbG9jayBzdWJ0cmVlIHNob3VsZCBiZSBtb2RlbGVkIGlu
IGEgd2F5DQo+IHRoYXQgaXMgY29udmVuaWVudCBmb3Igc29mdHdhcmUsIGJ1dCBpbnN0ZWFkIG1v
ZGVsIHRoZSBhY3R1YWwgaGFyZHdhcmUuDQo+IFRydXN0IG1lLCBpZiB5b3UgZG9uJ3QgbW9kZWwg
dGhlIGFjdHVhbCBoYXJkd2FyZSB0aGVuIHlvdSB3aWxsIGJlIHZlcnkNCj4gY29uZnVzZWQgd2hl
biB5b3UgY29tZSBiYWNrIGFuZCByZXZpc2l0IHRoaXMgY29kZSBpbiA2IG1vbnRocyBhbmQgY2Fu
J3QNCj4gcmVtZW1iZXIgd2h5IHRoaW5ncyBhcmUgc28gd2VpcmQgbG9va2luZy4NCg0KT2ssIHRo
ZW4gaXQgc2VlbXMgYW4gb21hcCBjbG9jayBlbnRyeSBmb3IgY29uLWlkICJpY2siIHNob3VsZCBi
ZSBjcmVhdGVkDQphcyBmb2xsb3dzIChkcGxsX2NvcmVfbTRfY2sgc3VwcGxpZXMgaW50ZXJmYWNl
IGNsb2NrKSwNCg0KQ0xLKCI0ODMwZTAwMC5sY2RjIiwgICAgImljayIsICAgICAgICAgICZkcGxs
X2NvcmVfbTRfY2ssICAgICAgIENLX0FNMzNYWCkNCg0KQW5kIHRoZW4gaW4gdGhlIGRyaXZlciwg
RE1BIGdhdGUgY2xvY2sgc2hvdWxkIGJlIG1hZGUgYSBjaGlsZCBvZiB0aGlzIGNsb2NrDQoob2J0
YWluZWQgd2l0aCBjb24taWQgImljayIpLg0KDQpMZXQgbWUga25vdyB5b3VyIG9waW5pb24gb24g
dGhpcy4NCg0KUmVnYXJkcw0KQWZ6YWwNCg0KDQoNCg=

Re: RE: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mike Turquette]

Quoting Mohammed, Afzal (2013-01-25 04:05:44)
> Hi Mike,
> 
> On Thu, Jan 24, 2013 at 22:30:44, Mike Turquette wrote:
> > Quoting Mohammed, Afzal (2013-01-24 03:36:02)
> 
> > > So there are 3 - LIDD is actually not for present use case, CORE could
> > > be clubbed with the divider to have a composite clock. And CORE is
> > > in functional clock path and logically it's perfectly alright to have
> > > the composite clock.
> 
> > Some of the clock names are a bit generic, so a question that I'm going
> > to repeat throughout my response: "is this clock only inside of your
> > video IP ?"
> 
> Yes these three clocks are inside LCDC IP.
> 
> > Regarding the CORE clock, is this only inside of your IP or are you
> > referring to the SoC CORE clock which is driven by a DPLL and clocks
> > DDR and many other peripherals (often MMC, UART, etc)?
> 
> Sorry for the confusion, here CORE refers to clock inside LCDC IP. This
> CORE should not be confused with CORE PLL. Actually I used CORE so that
> it corresponds to the nomenclature in LCDC section of TRM.
> 
> > Note that this is from my past experience with OMAP, and I'm making an
> > assumption that the clock scheme between OMAP and Da Vinci/AM335x parts
> > isn't very different.
> 
> Additional detail: DaVinci doesn't have these 3 clocks controls available,
> so these three are required only on AM335x (which has IP version 2 )
> 
> > Is there a public TRM I can look at?  It would help me understand this
> > without having to ask you so many annoying questions ;)
> 
> No problem, http://www.ti.com/product/am3359
> 
> 
> > > And now we are left with DMA, this is actually in the interface clock
> > > path which driver in unaware. An option would be to have DMA clock
> > > as child of CORE plus divider composite clock, even though logically
> > > DMA can't be considered in the same path.
> 
> > Why is the driver unaware of the interface clk?  For instance OMAP3 had
> > separate fclk and iclk for IPs and drivers would call clk_enable on
> > both.  Or am I misunderstanding something?
> 
> HWMOD handles enabling those upon pm_runtime calls, HWMOD makes an alias
> for main clock with "fck", but not for "ick", so currently "ick" is
> unavailable for the driver, continued below ..
> 
> > In general I don't think the clock subtree should be modeled in a way
> > that is convenient for software, but instead model the actual hardware.
> > Trust me, if you don't model the actual hardware then you will be very
> > confused when you come back and revisit this code in 6 months and can't
> > remember why things are so weird looking.
> 
> Ok, then it seems an omap clock entry for con-id "ick" should be created
> as follows (dpll_core_m4_ck supplies interface clock),
> 
> CLK("4830e000.lcdc",    "ick",          &dpll_core_m4_ck,       CK_AM33XX)
> 
> And then in the driver, DMA gate clock should be made a child of this clock
> (obtained with con-id "ick").
> 
> Let me know your opinion on this.
> 

I think Paul W. or someone on the TI side should weigh in on your clkdev
entries.  My main point is that the actual tree should be modeled and
clocks shouldn't be globbed together unnecessarily.  As mentioned in the
other mail thread you might be better off making a divider for your LCDC
IP block and modeling each node individually.

Regards,
Mike

> Regards
> Afzal

Re: RE: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mike Turquette]

Quoting Mohammed, Afzal (2013-01-25 04:05:44)
> Hi Mike,
> 
> On Thu, Jan 24, 2013 at 22:30:44, Mike Turquette wrote:
> > Quoting Mohammed, Afzal (2013-01-24 03:36:02)
> 
> > > So there are 3 - LIDD is actually not for present use case, CORE could
> > > be clubbed with the divider to have a composite clock. And CORE is
> > > in functional clock path and logically it's perfectly alright to have
> > > the composite clock.
> 
> > Some of the clock names are a bit generic, so a question that I'm going
> > to repeat throughout my response: "is this clock only inside of your
> > video IP ?"
> 
> Yes these three clocks are inside LCDC IP.
> 
> > Regarding the CORE clock, is this only inside of your IP or are you
> > referring to the SoC CORE clock which is driven by a DPLL and clocks
> > DDR and many other peripherals (often MMC, UART, etc)?
> 
> Sorry for the confusion, here CORE refers to clock inside LCDC IP. This
> CORE should not be confused with CORE PLL. Actually I used CORE so that
> it corresponds to the nomenclature in LCDC section of TRM.
> 
> > Note that this is from my past experience with OMAP, and I'm making an
> > assumption that the clock scheme between OMAP and Da Vinci/AM335x parts
> > isn't very different.
> 
> Additional detail: DaVinci doesn't have these 3 clocks controls available,
> so these three are required only on AM335x (which has IP version 2 )
> 
> > Is there a public TRM I can look at?  It would help me understand this
> > without having to ask you so many annoying questions ;)
> 
> No problem, http://www.ti.com/product/am3359
> 
> 
> > > And now we are left with DMA, this is actually in the interface clock
> > > path which driver in unaware. An option would be to have DMA clock
> > > as child of CORE plus divider composite clock, even though logically
> > > DMA can't be considered in the same path.
> 
> > Why is the driver unaware of the interface clk?  For instance OMAP3 had
> > separate fclk and iclk for IPs and drivers would call clk_enable on
> > both.  Or am I misunderstanding something?
> 
> HWMOD handles enabling those upon pm_runtime calls, HWMOD makes an alias
> for main clock with "fck", but not for "ick", so currently "ick" is
> unavailable for the driver, continued below ..
> 
> > In general I don't think the clock subtree should be modeled in a way
> > that is convenient for software, but instead model the actual hardware.
> > Trust me, if you don't model the actual hardware then you will be very
> > confused when you come back and revisit this code in 6 months and can't
> > remember why things are so weird looking.
> 
> Ok, then it seems an omap clock entry for con-id "ick" should be created
> as follows (dpll_core_m4_ck supplies interface clock),
> 
> CLK("4830e000.lcdc",    "ick",          &dpll_core_m4_ck,       CK_AM33XX)
> 
> And then in the driver, DMA gate clock should be made a child of this clock
> (obtained with con-id "ick").
> 
> Let me know your opinion on this.
> 

I think Paul W. or someone on the TI side should weigh in on your clkdev
entries.  My main point is that the actual tree should be modeled and
clocks shouldn't be globbed together unnecessarily.  As mentioned in the
other mail thread you might be better off making a divider for your LCDC
IP block and modeling each node individually.

Regards,
Mike

> Regards
> Afzal

Re: RE: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mike Turquette]

Quoting Mohammed, Afzal (2013-01-25 04:05:44)
> Hi Mike,
> 
> On Thu, Jan 24, 2013 at 22:30:44, Mike Turquette wrote:
> > Quoting Mohammed, Afzal (2013-01-24 03:36:02)
> 
> > > So there are 3 - LIDD is actually not for present use case, CORE could
> > > be clubbed with the divider to have a composite clock. And CORE is
> > > in functional clock path and logically it's perfectly alright to have
> > > the composite clock.
> 
> > Some of the clock names are a bit generic, so a question that I'm going
> > to repeat throughout my response: "is this clock only inside of your
> > video IP ?"
> 
> Yes these three clocks are inside LCDC IP.
> 
> > Regarding the CORE clock, is this only inside of your IP or are you
> > referring to the SoC CORE clock which is driven by a DPLL and clocks
> > DDR and many other peripherals (often MMC, UART, etc)?
> 
> Sorry for the confusion, here CORE refers to clock inside LCDC IP. This
> CORE should not be confused with CORE PLL. Actually I used CORE so that
> it corresponds to the nomenclature in LCDC section of TRM.
> 
> > Note that this is from my past experience with OMAP, and I'm making an
> > assumption that the clock scheme between OMAP and Da Vinci/AM335x parts
> > isn't very different.
> 
> Additional detail: DaVinci doesn't have these 3 clocks controls available,
> so these three are required only on AM335x (which has IP version 2 )
> 
> > Is there a public TRM I can look at?  It would help me understand this
> > without having to ask you so many annoying questions ;)
> 
> No problem, http://www.ti.com/product/am3359
> 
> 
> > > And now we are left with DMA, this is actually in the interface clock
> > > path which driver in unaware. An option would be to have DMA clock
> > > as child of CORE plus divider composite clock, even though logically
> > > DMA can't be considered in the same path.
> 
> > Why is the driver unaware of the interface clk?  For instance OMAP3 had
> > separate fclk and iclk for IPs and drivers would call clk_enable on
> > both.  Or am I misunderstanding something?
> 
> HWMOD handles enabling those upon pm_runtime calls, HWMOD makes an alias
> for main clock with "fck", but not for "ick", so currently "ick" is
> unavailable for the driver, continued below ..
> 
> > In general I don't think the clock subtree should be modeled in a way
> > that is convenient for software, but instead model the actual hardware.
> > Trust me, if you don't model the actual hardware then you will be very
> > confused when you come back and revisit this code in 6 months and can't
> > remember why things are so weird looking.
> 
> Ok, then it seems an omap clock entry for con-id "ick" should be created
> as follows (dpll_core_m4_ck supplies interface clock),
> 
> CLK("4830e000.lcdc",    "ick",          &dpll_core_m4_ck,       CK_AM33XX)
> 
> And then in the driver, DMA gate clock should be made a child of this clock
> (obtained with con-id "ick").
> 
> Let me know your opinion on this.
> 

I think Paul W. or someone on the TI side should weigh in on your clkdev
entries.  My main point is that the actual tree should be modeled and
clocks shouldn't be globbed together unnecessarily.  As mentioned in the
other mail thread you might be better off making a divider for your LCDC
IP block and modeling each node individually.

Regards,
Mike

> Regards
> Afzal

RE: RE: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mohammed, Afzal]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1167 bytes --]

Hi Mike,

On Sat, Jan 26, 2013 at 04:14:53, Mike Turquette wrote:

> I think Paul W. or someone on the TI side should weigh in on your clkdev
> entries.  My main point is that the actual tree should be modeled and
> clocks shouldn't be globbed together unnecessarily.  As mentioned in the
> other mail thread you might be better off making a divider for your LCDC
> IP block and modeling each node individually.

It seems complexity of driver would increase by creating a new inherited
divider clock and having a total 3-4 clock nodes. The advantage going
with it would be higher configurable resolution for pixel clock.
Current use cases work without higher pixel clock resolution.

And drm driver posted for the same IP is without CCF modeling.

So I will presently not model clock nodes in LCDC IP, later if use cases
badly require, this can be done (and if it happens, hopefully by that
DaVinci would be CCF'ed and it would be more clean to implement it).

Thanks for sharing your ideas.

Regards
Afzal
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: RE: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mohammed, Afzal]

Hi Mike,

On Sat, Jan 26, 2013 at 04:14:53, Mike Turquette wrote:

> I think Paul W. or someone on the TI side should weigh in on your clkdev
> entries.  My main point is that the actual tree should be modeled and
> clocks shouldn't be globbed together unnecessarily.  As mentioned in the
> other mail thread you might be better off making a divider for your LCDC
> IP block and modeling each node individually.

It seems complexity of driver would increase by creating a new inherited
divider clock and having a total 3-4 clock nodes. The advantage going
with it would be higher configurable resolution for pixel clock.
Current use cases work without higher pixel clock resolution.

And drm driver posted for the same IP is without CCF modeling.

So I will presently not model clock nodes in LCDC IP, later if use cases
badly require, this can be done (and if it happens, hopefully by that
DaVinci would be CCF'ed and it would be more clean to implement it).

Thanks for sharing your ideas.

Regards
Afzal

RE: RE: RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mohammed, Afzal]

SGkgTWlrZSwNCg0KT24gU2F0LCBKYW4gMjYsIDIwMTMgYXQgMDQ6MTQ6NTMsIE1pa2UgVHVycXVl
dHRlIHdyb3RlOg0KDQo+IEkgdGhpbmsgUGF1bCBXLiBvciBzb21lb25lIG9uIHRoZSBUSSBzaWRl
IHNob3VsZCB3ZWlnaCBpbiBvbiB5b3VyIGNsa2Rldg0KPiBlbnRyaWVzLiAgTXkgbWFpbiBwb2lu
dCBpcyB0aGF0IHRoZSBhY3R1YWwgdHJlZSBzaG91bGQgYmUgbW9kZWxlZCBhbmQNCj4gY2xvY2tz
IHNob3VsZG4ndCBiZSBnbG9iYmVkIHRvZ2V0aGVyIHVubmVjZXNzYXJpbHkuICBBcyBtZW50aW9u
ZWQgaW4gdGhlDQo+IG90aGVyIG1haWwgdGhyZWFkIHlvdSBtaWdodCBiZSBiZXR0ZXIgb2ZmIG1h
a2luZyBhIGRpdmlkZXIgZm9yIHlvdXIgTENEQw0KPiBJUCBibG9jayBhbmQgbW9kZWxpbmcgZWFj
aCBub2RlIGluZGl2aWR1YWxseS4NCg0KSXQgc2VlbXMgY29tcGxleGl0eSBvZiBkcml2ZXIgd291
bGQgaW5jcmVhc2UgYnkgY3JlYXRpbmcgYSBuZXcgaW5oZXJpdGVkDQpkaXZpZGVyIGNsb2NrIGFu
ZCBoYXZpbmcgYSB0b3RhbCAzLTQgY2xvY2sgbm9kZXMuIFRoZSBhZHZhbnRhZ2UgZ29pbmcNCndp
dGggaXQgd291bGQgYmUgaGlnaGVyIGNvbmZpZ3VyYWJsZSByZXNvbHV0aW9uIGZvciBwaXhlbCBj
bG9jay4NCkN1cnJlbnQgdXNlIGNhc2VzIHdvcmsgd2l0aG91dCBoaWdoZXIgcGl4ZWwgY2xvY2sg
cmVzb2x1dGlvbi4NCg0KQW5kIGRybSBkcml2ZXIgcG9zdGVkIGZvciB0aGUgc2FtZSBJUCBpcyB3
aXRob3V0IENDRiBtb2RlbGluZy4NCg0KU28gSSB3aWxsIHByZXNlbnRseSBub3QgbW9kZWwgY2xv
Y2sgbm9kZXMgaW4gTENEQyBJUCwgbGF0ZXIgaWYgdXNlIGNhc2VzDQpiYWRseSByZXF1aXJlLCB0
aGlzIGNhbiBiZSBkb25lIChhbmQgaWYgaXQgaGFwcGVucywgaG9wZWZ1bGx5IGJ5IHRoYXQNCkRh
VmluY2kgd291bGQgYmUgQ0NGJ2VkIGFuZCBpdCB3b3VsZCBiZSBtb3JlIGNsZWFuIHRvIGltcGxl
bWVudCBpdCkuDQoNClRoYW5rcyBmb3Igc2hhcmluZyB5b3VyIGlkZWFzLg0KDQpSZWdhcmRzDQpB
ZnphbA0K