Re: [PATCH v3 01/12] clk: samsung: exynos5433: Add clocks using common clock framework

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Sylwester,

Do you have any comment about Tomasz and me reply?

Thanks,
Chanwoo Choi

On 01/24/2015 07:05 AM, Chanwoo Choi wrote:
> Hi Sylwester,
> 
> On Sat, Jan 24, 2015 at 2:40 AM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> On 23/01/15 08:44, Chanwoo Choi wrote:
>>>>> +   cmu_top: clock-controller@0x10030000 {
>>>>>>> +                compatible = "samsung,exynos5433-cmu-top";
>>>>>>> +                reg = <0x10030000 0x0c04>;
>>>>>>> +                #clock-cells = <1>;
>>>>>>> +        };
>>>>>>> +
>>
>>>>>>> +        cmu_fsys: clock-controller@0x156e0000 {
>>>>>>> +                compatible = "samsung,exynos5433-cmu-fsys";
>>>>>>> +                reg = <0x156e0000 0x0b04>;
>>>>>>> +                #clock-cells = <1>;
>>>>>>> +        };
>>>>>
>>>>> What are the reasons to split the whole clock controller into separate
>>>>> device nodes with different compatible strings like this? I doubt drivers
>>>>> associated with each of those compatible strings could be ever reused on
>>>>> different Exynos SoCs.
>>>
>>> No special reason. I added the clock controller according to clock domain
>>> separately. As I knew, samsung clk drivers use this way to support various
>>> clock domains. For exmaple, drivers/clk/samsung/clk-exynos7.c.
>>
>> I'm afraid exynos7 has that initialization ordering issue, unfortunately I
>> didn't notice it before.
> 
> OK.
> 
>>
>>>>> There are hardware dependencies between these clock domains, which are
>>>>> not currently modelled in DT with your binding.
>>>
>>> Right. current samsung clock drivers cannot show the hierarchy among clock
>>> domains in DT.
>>>
>>>>> IOW, there is currently
>>>>> no way to ensure proper registration order of the CMUs (clock domains).
>>>>> This may be important in some cases.
>>>>>
>>>>> To address this we could either add clocks/clock-names properties in
>>>>> respective CMU device nodes, pointing to any clocks in other CMU(s) or
>>>>> make a single device node for the whole clock controller, with an
>>>>> aggregated reg entry, e.g.
>>>>>
>>>>>  cmu: clock-controller@0x10030000 {
>>>>>    compatible = "samsung,exynos5433-cmu";
>>>>>    reg =   <0x10030000 0x0c04>,
>>>>>            <0x10fc0000 0x0c04>,
>>>>>            <0x105b0000 0x100c>,
>>>>>            <0x14c80000 0x0b08>,
>>>>>            <0x10040000 0x0b20>,
>>>>>            <0x156e0000 0x0b04>,
>>>>>                    ...
>>>>>    reg-names = "top", "cpif", "mif", "peric", "peris", "fsys"...
>>>>>    #clock-cells = <1>;
>>>>>  };
>>>
>>> If you make a single device node to support various clock domain,
>>> How are you indicate the specific clock in some clock domain?
>>
>> This might be an issue, we would need to make all the clk indexes a one
>> contiguous set.
> 
> Exynos5433 has a whole lot of clocks against Exynos4 series clocks.
> So, if make all the clocks in the same set, I wonder making too huge set.
> It may cause the complicated code to find the proper clock or to analyze
> the clock driver.
> 
> I'm wondering if there is really any use of having such
>> information expressed explicitly in DT, or it would just make the DT
>> binding closer resembling the SoC's documentation ?
> 
> If we show the hierarchy or dependency between clock domains,
> I think we should modify "structure samsung_clk_provider"
> to include dependency information between clock domains.
> (It is just my opinion, this opinion could be not proper solution.)
> 
> Because
> when we use the common clk framework without adding
> any dependency information between clock domains, it is well working.
> 
>>
>> Similarly, the clock controller is divided into subdomains in older SoCs,
>> like exynos4, yet we do not create separate device nodes for each domain.
>> Is reference to each individual clock domain required in any other SoC's
>> part in case of exynos5433 ?
> 
> There is a difference between exynos4 cmu and exynos5433 cmu.
> exynos4. As I knew, Exynos4 series have the one more clock domain.
> But, there are not any IPs between clock domains. We can check it as following
> read base address and scope.
> 
> The base address and range of Exynos4412 clock domain :
> - 0x1003_0000 ~ 0x1003_CA08
> - 0x1004_0000 ~ 0x1004_8B0C
> 
> But, the clock domain in base address map of exynos5433 is located
> in non-continuous range. Also, there are un-related IPs to clocks.
> (e.g., mct 101c_0000, gic 1100_1000, serial0 14c1_0000, pinctrl 1058_0000 ...)
> If we make the one dt node for clock domains like exynos4,
> I think it may cause the possible issue that clock drivers may access
> the un-related memory-mapped region.
> 
> The base address and range of Exynos5433 clock domain :
> - top domain    : 0x1003_0000 ~ 0x1003_0c04
> - cpif domain   : 0x10fc_0000 ~ 0x10fc_0x0c04
> - mif domain    : 0x105b_0000 ~ 0x105b_0x100c
> - peric domain : 0x14c8_0000 ~ 0x14c8_0b08
> - peris domain  : 0x1004_0000 ~ 0x1004_0x0b20
> - fsys domain   : 0x156e_0000 ~ 0x156e_0b04
> 
>>
>>> For example,
>>> The serial dt node in exynos7.dtsi. serial_0 dt node use the uart clocks
>>> in 'clock_peric0' clock domain and serial_1 dt node use the uart clocks
>>> in 'clock-peric1' clock domain.
>>>
>>> When using the clock in specific clock domain,
>>> we need to phandle(e.g., clock_peric0, clock_peric1) of clock domain.
>>>
>>>               serial_0: serial@13630000 {
>>>                       compatible = "samsung,exynos4210-uart";
>>>                       reg = <0x13630000 0x100>;
>>>                       interrupts = <0 440 0>;
>>>                       clocks = <&clock_peric0 PCLK_UART0>,
>>>                                <&clock_peric0 SCLK_UART0>;
>>>                       clock-names = "uart", "clk_uart_baud0";
>>>                       status = "disabled";
>>>               };
>>>
>>>               serial_1: serial@14c20000 {
>>>                       compatible = "samsung,exynos4210-uart";
>>>                       reg = <0x14c20000 0x100>;
>>>                       interrupts = <0 456 0>;
>>>                       clocks = <&clock_peric1 PCLK_UART1>,
>>>                                <&clock_peric1 SCLK_UART1>;
>>>                       clock-names = "uart", "clk_uart_baud0";
>>>                       status = "disabled";
>>>               };
>>>
>>>>> Then we could modify samsung_cmu_register_one() function by adding
>>>>> the reg entry index or name argument. What do you think ?
>>>
>>> If there is more reasonable way to show the dependency between clock domains,
>>> I will agree.
>>
>> The other option I mentioned is specifying all parent clocks of a given
>> clock domain in its device node with clocks(/clock-names) properties.
>> But it might be a bit hard to list all the clock domain dependencies
>> this way
> 
> Right, I agree that it is too hard.
> 
> Regards,
> Chanwoo Choi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>