[PATCH RFC/RFT] ARM: S5P: Fix USB and 48M clock enable procedure

[paulius.zaleckas@gmail.com (Paulius Zaleckas)]

On 10/15/2010 03:56 PM, Kukjin Kim wrote:
> Paulius Zaleckas wrote:
>>
>> On Thu, Oct 14, 2010 at 12:03 PM, Kukjin Kim<kgene.kim@samsung.com>
> wrote:
>>> Paulius Zaleckas wrote:
>>>>
>>>> Signed-off-by: Paulius Zaleckas<paulius.zaleckas@gmail.com>
>>>> ---
>>>>
>>>>   arch/arm/mach-s5p6440/clock.c              |   23 -------
>>>>   arch/arm/plat-s5p/clock.c                  |   86
>>>> ++++++++++++++++++++++++++++
>>>>   arch/arm/plat-s5p/include/plat/s5p-clock.h |    1
>>>>   3 files changed, 86 insertions(+), 24 deletions(-)
>>>>
>>> Hi Paulius,
>>>
>>> There are some comments about your patches which includes previous
> S3C64XX
>> patches.
>>>
>>> Basically your approach looks good trial and structure...but I'm not
> sure
>> whether your approach can be used commonly on Samsung's all SoCs or not.
>>> Need to do more test on boards and I already informed your patches to
> USB
>> engineers in my team, actually need to discuss about this.
>>>
>>> As a note, I know, 'xusbxti' clock is structure for external xtal which
> is
>> used for generating USB clock on board... it depends on board condition,
>> because can be used 12/24/48Mhz on board. The clk_48m means generated
> actual
>> USB clock, 48Mhz. So should be implemented enable function by using
> clk_48m...
>>
>> I don't agree that enable should be for clk_48m. The reason is that
>> IMO it is possible
>> to enable 48m clock, but suspend the clock for USB device part (I am
>> not sure about this yet...).
> 
> Umm...please see below.
> 
>            |\
> xxti------|O|
>            |M|-----..System..-- HCLK -->  usb_device...
> xusbxti---| |  |
>          | |/   |
>          |      |
>         (1)    (2)  -------------
>           ---------| USB OTG Phy | -->  clk_48M
>                     -------------
> 
> Firstly, 'xusbxti' can be used to system clock when selected by OM pin.
> As I said, the rate of xusbxti can be 12/24/48Mhz and there is it on
> board...usually is used 12Mhz or 24Mhz on SMDK board.
> It means depends on board and always enabled, so no need following 'rate'
> and 'enable' in your following patch.
> 
>   struct clk clk_xusbxti = {
>   	.name		= "xusbxti",
>   	.id		= -1,
> +	.rate		= 48000000,
> +	.enable		= clk_xusbxti_ctrl,
>   };
> 
>   struct clk s5p_clk_27m = {
> @@ -49,6 +134,7 @@ struct clk clk_48m = {
>   	.name		= "clk_48m",
>   	.id		= -1,
>   	.rate		= 48000000,
> +	.parent		=&clk_xusbxti,
>   };
> 
> According to above clock diagram, the parent of 'clk_48m' can be 'xxti' or
> 'xusbxti', it is decided by SoC and OM pin. For example, there is line (1)
> on S5P6440 so your patch is right, but there is line (2) on S5PC100 but
> line (1) so it's wrong..the parent of clk_48m can be xxti by OM Pin.
> 
> It means the parent of clk_48m depends on SoC, can't define it in the plat-
> s5p. And maybe as you know, in the S5PV210/S5PC110 case, need two clk_48m
> structures for Host and Device...
> 
> So its clock structure has SoC(ARCH) dependency and it's difficult to merge
> it into plat-s5p now.

I see... So maybe the best option is to define enable function not per
platform, but per machine.

Originally I was trying to solve bug where OHCI was not working on
S3C6410. I discovered that I need to select different clock source for
it or fix enable procedure for 48M. For S5P platform I have datasheet
only for S5PC100 and yes you are right about clock structure there.

> We need to re-think to implement it...and I and my team will sort out it.

Looking forward!

>>
>> If that is true than I think we will need one more clk for USB device:
>>
>>          /->clk_48m
>> xusbxti-|
>>          \->clk_usb_device
>>
> Actually, it's different on each SoCs...
> 
>>> Anyway let you know about the result of internal discussion soon, then
>> let's talk.
>>>
> 
> Thanks.
> 
> Best regards,
> Kgene.
> --
> Kukjin Kim<kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
> 
>