Re: Re: [PATCH v2 1/5] ARM: EXYNOS4: Change clock name for FIMD

[JinGoo Han <jg1.han@samsung.com>]

> -----Original Message-----
> From: Sylwester Nawrocki [mailto:s.nawrocki@samsung.com]
> Sent: Monday, June 20, 2011 6:48 PM
> To: jg1.han@samsung.com
> Cc: Sylwester Nawrocki; Kukjin Kim; Paul Mundt; linux-samsung-
> soc@vger.kernel.org; Jong-Hun Han; ANAND KUMAR N; THOMAS P ABRAHAM; Marek
> Szyprowski; Kyungmin Park; In-Ki Dae; ARM Linux; Ben Dooks
> Subject: Re: [PATCH v2 1/5] ARM: EXYNOS4: Change clock name for FIMD
> 
> Hi JinGoo,
> 
> On 06/20/2011 09:14 AM, JinGoo Han wrote:
> > Hi, Sylwester Nawrocki.
> > I appreciate your review and suggestion.
> >
> > Please, refer to the LCD contoller clock table as follows:
> 
> ok, thanks for the update.
> 
> >  - s3c2440 uses 's3c2410fb.c', not 's3c-fb.c' since  LCD controller IP is
> different.
> >    However, s3c2443 uses 's3c-fb.c'. So I add s3c2443 to table instead of
> s3c2440.
> 
> Yes, I was aware of that. My bad to put s3c2440 in the table.
> 
> >  - s3c6410 has SCLK_LCD, but, clock name is not defined.
> >  - Exynos4 does not use name "HCLK".
> >
> >           | LCD controller    |                            |
> >           | (IP core) clock   | LCD pixel clock            |
> > ----------+------------------------+-----------------------+
> > s3c2443   |  HCLK (lcd)       | x  |  DISPCLK (display-if) |
> > ----------+------------------------+-----------------------+
> > s3c6410   |  HCLK (lcd)       | x  |  SCLK_LCD  (N/A)      |
> > ----------+------------------------+-----------------------+
> > s5pc100   |  HCLK (lcd)       | x  |  SCLK_LCD  (sclk_lcd) |
> > ----------+------------------------+-----------------------+
> > s5pv210   |  HCLK_DSYS (lcd)  | x  |  SCLK_FIMD (sclk_fimd)|
> > ----------+-----------------------+------------------------+
> > exynos4   |  ACLK_160 (fimd)  | O  |  SCLK_FIMD (sclk_fimd)|
> > ----------+------------------------+-----------------------+
>              ^^^^^^^^^^^^^^^^^^^
> In mach-exynos4/clock.c this clock is described as ACLK_133 (lcd)
I cannot find it.
Let me know where 'fimd' is described as ACLK_133.
Anyway, according to datasheet, this clock is described as ACLK_160.
> 
> >
> > s3c2443, s3c6410, s5pc100 and s5pv210 don't use 'sclk_lcd' or
> 'sclk_fimd'.
> > 'lcd' clock is also used to generate the LCD pixel clock.
> >
> > My point is that LCD controller clock should be named "lcd" for
> consistence.
> 
> Yes, I agree. After thinking about it a bit more I was going to propose
> that too.
> 
> > If there is not mux for lcd pixel clock in case of exynos4, "sclk_fimd"
> will be set
> > in machine directory.
> 
> OK, you patch for s3c-fb driver looks like a significant improvement
> comparing
> to the original one. But I think we should remove the callback into
> machine
> code.
> The driver could just directly be doing clk_get(dev, "sclk_fimd"); If this
> succeeds and clksel option is not set in the IP variant then the driver
> should
> treat "sclk_fimd" as pixel clock, i.e. it will set its frequency and
> enable it.
> It should not care about setting the parent for "sclk_fimd", this should
> be done before s3c-fb probe is called.
> 
> The problem is that I don't know what to do it the bootloader does not set
> a parent clock for sclk_fimd..
> The board code could just get sclk_fimd and set mout_mpll as its parent,
> like
> it's done in your patch:
> [PATCH v2 3/5] ARM: EXYNOS4: Add platform device and helper functions for
> FIMD
> (except passing a pointer to the driver).
> 
> However there have been objections to put such things in the board code in
> the past.
> In case of camera clocks we used to have internally a function in the
> machine
> file setting the parent clocks, until bootloader was modified to configure
> them.
> 
> >
> > As you mentioned, I also think that we need to create two clock
> connection ids
> > such as  "bus_ck", "pix_ck" in order to use SCLK_LCD or SCLK_FIMD.
> > Moreover, 'lcd' in s5pv210 should be changed to 'fimd' according to
> s5pv210 datasheet.
> 
> Yeah, that makes sense.
> 
> > However, it requires many works to convert.
> 
> It's a bit laborious. But it's doable.
> 
> >
> > So, I think that 'two clock connection ids' patch would be submitted
> later,
> > after committing the patches that I submitted on last Friday.
> 
> I agree with that, given that the callback is removed from the platform
> data
> structure.
OK. The callback will be removed from the platform data structure.
> We need to get ourselves onto path of migration to the device tree and
> IMHO
> adding more callbacks to board code is a step in opposite direction.
> 
> 
> Thanks,
> S.