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

[Tomasz Figa]

2015-01-23 1:47 GMT+09:00 Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>:
> Hi Chanwoo,
>
> On 21/01/15 07:26, Chanwoo Choi wrote:
>> This patch adds the support for CMU (Clock Management Units) of Exynos5433
>> which is 64bit SoC and has Octa-cores. This patch supports necessary clocks
>> (PLL/MMC/UART/MCT/I2C/SPI) for kernel boot and includes binding documentation
>> for Exynos5433 clock controller.
>
>> diff --git a/Documentation/devicetree/bindings/clock/exynos5433-clock.txt b/Documentation/devicetree/bindings/clock/exynos5433-clock.txt
>> new file mode 100644
>> index 0000000..72cd0ba
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/exynos5433-clock.txt
>> @@ -0,0 +1,106 @@
>> +* Samsung Exynos5433 CMU (Clock Management Units)
>> +
>> +The Exynos5433 clock controller generates and supplies clock to various
>> +controllers within the Exynos5433 SoC.
>> +
>> +Required Properties:
>> +
>> +- compatible: should be one of the following.
>> +  - "samsung,exynos5433-cmu-top"   - clock controller compatible for CMU_TOP
>> +    which generates clocks for IMEM/FSYS/G3D/GSCL/HEVC/MSCL/G2D/MFC/PERIC/PERIS
>> +    domains and bus clocks.
>> +  - "samsung,exynos5433-cmu-cpif"  - clock controller compatible for CMU_CPIF
>> +    which generates clocks for LLI (Low Latency Interface) IP.
>> +  - "samsung,exynos5433-cmu-mif"   - clock controller compatible for CMU_MIF
>> +    which generates clocks for DRAM Memory Controller domain.
>> +  - "samsung,exynos5433-cmu-peric" - clock controller compatible for CMU_PERIC
>> +    which generates clocks for UART/I2C/SPI/I2S/PCM/SPDIF/PWM/SLIMBUS IPs.
>> +  - "samsung,exynos5433-cmu-peris" - clock controller compatible for CMU_PERIS
>> +    which generates clocks for PMU/TMU/MCT/WDT/RTC/SECKEY/TZPC IPs.
>> +  - "samsung,exynos5433-cmu-fsys"  - clock controller compatible for CMU_FSYS
>> +    which generates clocks for USB/UFS/SDMMC/TSI/PDMA IPs.
>> +
>> +- reg: physical base address of the controller and length of memory mapped
>> +  region.
>> +
>> +- #clock-cells: should be 1.
>> +
>> +Each clock is assigned an identifier and client nodes can use this identifier
>> +to specify the clock which they consume.
>> +
>> +All available clocks are defined as preprocessor macros in
>> +dt-bindings/clock/exynos5433.h header and can be used in device
>> +tree sources.
>> +
>> +Example 1: Examples of clock controller nodes are listed below.
>> +
>> +     cmu_top: clock-controller@0x10030000 {
>> +             compatible = "samsung,exynos5433-cmu-top";
>> +             reg = <0x10030000 0x0c04>;
>> +             #clock-cells = <1>;
>> +     };
>> +
>> +     cmu_cpif: clock-controller@0x10fc0000 {
>> +             compatible = "samsung,exynos5433-cmu-cpif";
>> +             reg = <0x10fc0000 0x0c04>;
>> +             #clock-cells = <1>;
>> +     };
>> +
>> +     cmu_mif: clock-controller@0x105b0000 {
>> +             compatible = "samsung,exynos5433-cmu-mif";
>> +             reg = <0x105b0000 0x100c>;
>> +             #clock-cells = <1>;
>> +     };
>> +
>> +     cmu_peric: clock-controller@0x14c80000 {
>> +             compatible = "samsung,exynos5433-cmu-peric";
>> +             reg = <0x14c80000 0x0b08>;
>> +             #clock-cells = <1>;
>> +     };
>> +
>> +     cmu_peris: clock-controller@0x10040000 {
>> +             compatible = "samsung,exynos5433-cmu-peris";
>> +             reg = <0x10040000 0x0b20>;
>> +             #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.

Well, they are separate IP blocks, so this is how they should be
represented in DT and IMHO using different compatible strings is a bit
cleaner than relying on aliases to get instance index.

Plus we probably want to learn from mistakes done when designing
Exynos4x12 clock bindings, which prevent us from fixing the issue of
ISP power domain, in which the ISP CMU is located. AFAICS, Exynos5433
clock tree is split between several such "ISP CMUs".

>
> There are hardware dependencies between these clock domains, which are
> not currently modelled in DT with your binding. 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)

which I believe is the proper way of doing things, which lets us
preserve the correct representation of separate CMUs in DT.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/