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

[Sylwester Nawrocki <snjw23@gmail.com>]

Hi Inki

On 06/20/2011 12:09 PM, daeinki wrote:
> Hi, Mr. Han and Sylwester.
> below is my opinion.
> 
> JinGoo Han 쓴 글:
...
>> Please, refer to the LCD contoller clock table as follows:
>>   - 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.
>>   - 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)|
>> ----------+------------------------+-----------------------+
...
>>> I think we could try to create two clock connection ids to the framebuffer
>>> device in the first place, e.g. "bus_ck", "pix_ck".
>>> And then think about how handle that in the driver.
>>>
>>> But this requires conversion to the omap-style clock registration method,
>>> something like in the attached patch. The patch is only for s5pv210 and
>>> and compile tested only as I didn't have any board to test it here.
>>> It's based on for-next branch at http://tinyurl.com/6yzravy I think there
>>> might be more issues to convert the old s3c24xx platforms, nevertheless
>>> the attached patch should not affect them.
...

> when someone adds new board file with new SoC, he doesn't need to know
> this SoC chip has hclk and sclk_fimd or only sclk_fimd(such as exynos4).
> using implicit clock means it should know that this SoC chip has both
> clocks(bus clock, sclk_fimd) or only sclk_fimd.
> 
> for example, if any driver needs fimd clock frequency then this driver
> should know that this SoC chip is exynos4 or not and has both clock
> source(bus clock, soure clock fimd) or not(only source clock fimd)
> so I think we shoule see only a clock "lcd" regardless of which clock is
> used and if exynos4 then sclk_fimd would be set by machine code.
> 
> and Sylwester,
> it appears that your patch has one issue about clk_get function call.
> your patch adds "bus_ck" to list head "clocks" of plat-samsung/clock.c

Why do you think so ? In fact the "clocks" list in plat-samsung/clock.c
is not used any more, AFAIU it should have been removed altogether with
clk_get/clk_put functions in Thomas' clkdev patches. 
When you comment out the line declaring the list everything compiles fine
there.

> and "pix_ck" to list head "clocks" of drivers/clk/clkdev.c and I am
> afraid that if some machine(such as s3c24xx, s3c64xx and s5pc1xx) has
> CLKDEV_LOOKUP configuration then clk_get() would fail to get clock
> object because in this case, clock lookup could be done through list
> head "clocks" of driver/clk/clkdev.c.(it's right from
> plat-samsung/clock.c) so I think it needs more patch for resolving this

As I indicated earlier the framebuffer driver would have to be modified
to support newly introduced clock _connection_ names.
We could (temporarily) name one of those clock connections "lcd",
to avoid additional trouble on SoCs that still use a one-to-one
platform clock name <-> clock connection id mapping.

> issue also and do you think it's a good way to use only one clock name
> "lcd"?... in fact, this might be so.... much slight issue. :)

Do you mean using using one name in the code for different clock names
in the datasheets ? I suppose it was because of the API limitations and
hope it will change for the better. :)

Cheers,
Sylwester