From: Afzal Mohammed <hidden> Date: 2013-01-23 11:48:09
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 [off-list ref]
2. Patch "da8xx: Allow use by am33xx based devices" by,
Pantelis Antoniou [off-list ref]
3. Series v3 "video: da8xx-fb: runtime timing configuration" by,
me (Afzal Mohammed [off-list ref])
To test this series on AM335x based boards,
1. Series v2 "ARM: dts: AM33XX: lcdc support" by,
me (Afzal Mohammed [off-list ref]),
2. Series "HWMOD fixes for AM33xx PWM submodules and device tree nodes" by,
Philip, Avinash [off-list ref]
3. Series v2 "clk: divider: prepare for minimum divider" by,
me (Afzal Mohammed [off-list ref]),
4. Series v2 "ARM: AM335x: LCDC platform support" by,
me (Afzal Mohammed [off-list ref]),
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
From: Afzal Mohammed <hidden> Date: 2013-01-23 11:48:22
interrupt handler is checking for sync lost interrupt, but it was not
enabled, enable it.
Signed-off-by: Afzal Mohammed <redacted>
---
drivers/video/da8xx-fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -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));}return0;
@@ -1265,7 +1262,6 @@ static int fb_probe(struct platform_device *device)structfb_info*da8xx_fb_info;structclk*fb_clk=NULL;structda8xx_fb_par*par;-resource_size_tlen;intret,i;unsignedlongulcm;
@@ -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;-gotoerr_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;-gotoerr_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)gotoirq_freq;
From: Afzal Mohammed <hidden> Date: 2013-01-23 11:48:38
Move panel detection to a separate function, this helps in readability
as well as makes DT support cleaner.
Signed-off-by: Afzal Mohammed <redacted>
---
drivers/video/da8xx-fb.c | 42 ++++++++++++++++++++++++++----------------
1 file changed, 26 insertions(+), 16 deletions(-)
From: Afzal Mohammed <hidden> Date: 2013-01-23 11:48:43
Driver is provided a means to have the probe triggered by DT.
Signed-off-by: Afzal Mohammed <redacted>
---
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
From: Afzal Mohammed <hidden> Date: 2013-01-23 11:48:49
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 <redacted>
---
drivers/video/da8xx-fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -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:
From: Afzal Mohammed <hidden> Date: 2013-01-23 11:48:59
This driver is DT probe-able, hence ensure presence of platform data
only for non-DT boot.
Signed-off-by: Afzal Mohammed <redacted>
---
drivers/video/da8xx-fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -1303,7 +1303,7 @@ static int fb_probe(struct platform_device *device)intret;unsignedlongulcm;-if(fb_pdata=NULL){+if(fb_pdata=NULL&&!device->dev.of_node){dev_err(&device->dev,"Can not get platform data\n");return-ENOENT;}
From: Afzal Mohammed <hidden> Date: 2013-01-23 11:49:06
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 <redacted>
---
drivers/video/da8xx-fb.c | 34 +++++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
From: Afzal Mohammed <hidden> Date: 2013-01-23 11:49:11
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 <redacted>
---
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(-)
@@ -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;+gotoerr_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);
From: Afzal Mohammed <hidden> Date: 2013-01-23 11:51:48
From: "Manjunathappa, Prakash" <redacted>
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 <redacted>
Signed-off-by: Afzal Mohammed <redacted>
---
drivers/video/da8xx-fb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Mike Turquette <hidden> Date: 2013-01-23 20:22:14
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
+ 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
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 ;)].
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
From: Mike Turquette <hidden> Date: 2013-01-25 22:45:05
Quoting Mohammed, Afzal (2013-01-25 04:05:44)
Hi Mike,
On Thu, Jan 24, 2013 at 22:30:44, Mike Turquette wrote:
quoted
Quoting Mohammed, Afzal (2013-01-24 03:36:02)
quoted
quoted
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.
quoted
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
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
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
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.
quoted
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
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