Skip to contentSkip to conversation

video: da8xx-fb: am335x DT support

PATCH Superseded Afzal Mohammed 2 participants 19 messages Jan 23, 2013 – Jan 28, 2013

Subsystems (2)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v4 cover video: da8xx-fb: am335x DT support

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 <[email protected]>
2. Patch "da8xx: Allow use by am33xx based devices" by, Pantelis Antoniou <[email protected]>
3. Series v3 "video: da8xx-fb: runtime timing configuration" by, me (Afzal Mohammed <[email protected]>)

To test this series on AM335x based boards,
1. Series v2 "ARM: dts: AM33XX: lcdc support" by,
me (Afzal Mohammed <[email protected]>),
2. Series "HWMOD fixes for AM33xx PWM submodules and device tree nodes" by, Philip, Avinash <[email protected]>
3. Series v2 "clk: divider: prepare for minimum divider" by,
me (Afzal Mohammed <[email protected]>),
4. Series v2 "ARM: AM335x: LCDC platform support" by,
me (Afzal Mohammed <[email protected]>),
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
Signature · 1 line
1.7.12
#1 lore raw

PATCH v4 1/12 video: da8xx-fb: make io operations safe

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

---

v2: new patch

 drivers/video/da8xx-fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/video/da8xx-fb.c +2 -2
index 720604c..35a33ca 100644
@@ -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 {
Signature · 1 line
1.7.12
#2 lore raw

PATCH v4 2/12 video: da8xx-fb: fix 24bpp raster configuration

In reply to Afzal Mohammed

From: "Manjunathappa, Prakash" <[email protected]>

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.

---
 drivers/video/da8xx-fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/video/da8xx-fb.c +2 -2
index 35a33ca..7f92f37 100644
@@ -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:
Signature · 1 line
1.7.12
#3 lore raw

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

In reply to Afzal Mohammed

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

---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/video/da8xx-fb.c +1 -1
index 7f92f37..ca69e01 100644
@@ -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;
Signature · 1 line
1.7.12
#4 lore raw

PATCH v4 4/12 video: da8xx-fb: use devres

In reply to Afzal Mohammed

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

---
 drivers/video/da8xx-fb.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)
drivers/video/da8xx-fb.c +6 -29
index ca69e01..7a32e83 100644
@@ -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;
 }
 
Signature · 1 line
1.7.12
#5 lore raw

PATCH v4 5/12 video: da8xx-fb: ensure non-null cfg in pdata

In reply to Afzal Mohammed

Ensure that platform data contains pointer for lcd_ctrl_config.

---
 drivers/video/da8xx-fb.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/video/da8xx-fb.c +5 -0
index 7a32e83..3b146bc 100644
@@ -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) {
Signature · 1 line
1.7.12
#6 lore raw

PATCH v4 6/12 video: da8xx-fb: reorganize panel detection

In reply to Afzal Mohammed

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

---
 drivers/video/da8xx-fb.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)
drivers/video/da8xx-fb.c +26 -16
index 3b146bc..b6ea5e9 100644
@@ -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) {
Signature · 1 line
1.7.12
#7 lore raw

PATCH v4 7/12 video: da8xx-fb: minimal dt support

In reply to Afzal Mohammed

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

---
 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
Documentation/devicetree/bindings/video/fb-da8xx.txt new file +16 -0
new file mode 100644
index 0000000..581e014
@@ -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>;
+};
drivers/video/da8xx-fb.c +7 -0
index b6ea5e9..08ee8eb 100644
@@ -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),
 		   },
 };
 
Signature · 1 line
1.7.12
#8 lore raw

PATCH v4 8/12 video: da8xx-fb: invoke platform callback safely

In reply to 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.

---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/video/da8xx-fb.c +1 -1
index 08ee8eb..0beed20 100644
@@ -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);
 	}
Signature · 1 line
1.7.12
#9 lore raw

PATCH v4 9/12 video: da8xx-fb: obtain fb_videomode info from dt

In reply to Afzal Mohammed

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

---
 .../devicetree/bindings/video/fb-da8xx.txt          | 21 +++++++++++++++++++++
 drivers/video/da8xx-fb.c                            | 17 +++++++++++++++++
 2 files changed, 38 insertions(+)
Documentation/devicetree/bindings/video/fb-da8xx.txt +21 -0
index 581e014..0741f78 100644
@@ -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>;
+		};
+	};
 };
drivers/video/da8xx-fb.c +17 -0
index 0beed20..0c68712 100644
@@ -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)
Signature · 1 line
1.7.12
#10 lore raw

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

In reply to Afzal Mohammed

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

---
 drivers/video/da8xx-fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/video/da8xx-fb.c +1 -1
index 0c68712..1c1a616 100644
@@ -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;
 	}
Signature · 1 line
1.7.12
#11 lore raw

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

In reply to 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.

---
 drivers/video/da8xx-fb.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)
drivers/video/da8xx-fb.c +33 -1
index 1c1a616..5455682 100644
@@ -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;
Signature · 1 line
1.7.12
#12 lore raw

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

In reply to 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.

---

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(-)
drivers/video/da8xx-fb.c +70 -2
index 5455682..6723683 100644
@@ -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);
Signature · 1 line
1.7.12
#13 lore raw

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

Quoting Afzal Mohammed (2013-01-23 03:48:56)
<snip>

Quoted 11 lines
drivers/video/da8xx-fb.c +5 -1
+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)

drivers/video/da8xx-fb.c static inline void da8xx_fb_config_clk_divider(unsigned div) · line 751

Quoted 10 lines
drivers/video/da8xx-fb.c +10 -0
+{
+       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

#14 lore raw

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

Hi Mike,

Quoted 20 lines · Mike Turquette

Quoting Afzal Mohammed (2013-01-23 03:48:56)

Quoted 7 lines
drivers/video/da8xx-fb.c +3 -0
+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,

drivers/video/da8xx-fb.c static inline void da8xx_fb_config_clk_divider(unsigned div) · line 761

Quoted 10 lines
drivers/video/da8xx-fb.c +9 -0
+                                                   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();
Quoted 4 lines

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

Quoted 3 lines

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

#15 lore raw

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

Quoting Mohammed, Afzal (2013-01-24 03:36:02)

Quoted 40 lines

Hi Mike,

Quoted 20 lines · Mike Turquette

Quoting Afzal Mohammed (2013-01-23 03:48:56)

Quoted 7 lines
drivers/video/da8xx-fb.c +3 -0
+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,

drivers/video/da8xx-fb.c static inline void da8xx_fb_config_clk_divider(unsigned div) · line 761

Quoted 10 lines
drivers/video/da8xx-fb.c +9 -0
+                                                   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();
Quoted 4 lines

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

Quoted 3 lines

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,

Quoted 8 lines

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

Quoted 5 lines

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

Quoted 10 lines

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

#16 lore raw

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

Hi Mike,

Quoted 10 lines · Mike Turquette

Quoting Mohammed, Afzal (2013-01-24 03:36:02)

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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 )

Quoted 2 lines

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

Quoted 8 lines
Quoted 4 lines

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

Quoted 5 lines

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

#17 lore raw

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

Quoting Mohammed, Afzal (2013-01-25 04:05:44)

Quoted 59 lines

Hi Mike,

Quoted 10 lines · Mike Turquette

Quoting Mohammed, Afzal (2013-01-24 03:36:02)

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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 )

Quoted 2 lines

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

Quoted 8 lines
Quoted 4 lines

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

Quoted 5 lines

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

Quoted 7 lines

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

Quoted 2 lines

Regards
Afzal

#18 lore raw

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

Hi Mike,

Quoted 5 lines · Mike Turquette

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