Re: [PATCH v11 3/4] thermal: samsung: Add TMU support for Exynos5420 SoCs

Tomasz Figa
2013-12-18T15:50:17Z

Hi Naveen,

On Tuesday 10 of December 2013 12:12:25 Naveen Krishna Chatradhi wrote:
> Exynos5420 has 5 TMU channels, the TRIMINFO register is
> misplaced for TMU channels 2, 3 and 4
> TRIMINFO at 0x1006c000 contains data for TMU channel 3
> TRIMINFO at 0x100a0000 contains data for TMU channel 4
> TRIMINFO at 0x10068000 contains data for TMU channel 2
[snip]
> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
> index a1aa602..a3e78c0 100644
> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
> @@ -6,6 +6,9 @@
> "samsung,exynos4412-tmu"
> "samsung,exynos4210-tmu"
> "samsung,exynos5250-tmu"
> + "samsung,exynos5420-tmu" for TMU channel 0, 1 on Exynos5420
> + "samsung,exynos5420-tmu-ext-triminfo" for TMU channels 2, 3 and 4
> + Exynos5420 (Must pass triminfo base and triminfo clock)

Exact clock names must be specified in description of clock-names property.

> "samsung,exynos5440-tmu"
> - interrupt-parent : The phandle for the interrupt controller
> - reg : Address range of the thermal registers. For soc's which has multiple
> @@ -13,6 +16,16 @@
> interrupt related then 2 set of register has to supplied. First set
> belongs to register set of TMU instance and second set belongs to
> registers shared with the TMU instance.
> +
> + NOTE: On Exynos5420, the TRIMINFO register is misplaced for TMU
> + channels 2, 3 and 4
> + Use "samsung,exynos5420-tmu-ext-triminfo" in cases, there is a misplaced
> + register, also provide clock to access that base.
> +
> + TRIMINFO at 0x1006c000 contains data for TMU channel 3
> + TRIMINFO at 0x100a0000 contains data for TMU channel 4
> + TRIMINFO at 0x10068000 contains data for TMU channel 2
> +
> - interrupts : Should contain interrupt for thermal system
> - clocks : The main clock for TMU device
> - clock-names : Thermal system clock name
> @@ -43,6 +56,31 @@ Example 2):
> clock-names = "tmu_apbif";
> };
>
> +Example 3): (In case of Exynos5420 "with misplaced TRIMINFO register")
> + tmu_cpu2: tmu@10068000 {
> + compatible = "samsung,exynos5420-tmu-ext-triminfo";
> + reg = <0x10068000 0x100>, <0x1006c000 0x4>;
> + interrupts = <0 184 0>;
> + clocks = <&clock 318>, <&clock 318>;
> + clock-names = "tmu_apbif", "tmu_triminfo_apbif";

Here you have "tmu_triminfo_apbif" clock, but in the driver you call
clk_get() with "tmu_apbif_triminfo".

> + };
> +
[snip]
>
> + data->clk_sec = devm_clk_get(&pdev->dev, "tmu_apbif_triminfo");

Here you try to get "tmu_apbif_triminfo" clock, but in binding
documentation you have "tmu_triminfo_apbif" in example.

> + if (IS_ERR(data->clk_sec)) {
> + if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
> + dev_err(&pdev->dev, "Failed to get triminfo clock\n");
> + return PTR_ERR(data->clk_sec);
> + }
> + } else {
> + ret = clk_prepare(data->clk_sec);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to get clock\n");
> + return ret;
> + }
> + }
> +
> ret = clk_prepare(data->clk);
> - if (ret)
> - return ret;
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to get clock\n");
> + goto err_clk_sec;
> + }
>
> if (pdata->type == SOC_ARCH_EXYNOS4210 ||
> pdata->type == SOC_ARCH_EXYNOS4412 ||
> pdata->type == SOC_ARCH_EXYNOS5250 ||
> + pdata->type == SOC_ARCH_EXYNOS5420_TRIMINFO ||

Don't you also need SOC_ARCH_EXYNOS5420 here?

> pdata->type == SOC_ARCH_EXYNOS5440)
> data->soc = pdata->type;
> else {
> @@ -703,6 +742,9 @@ static int exynos_tmu_probe(struct platform_device *pdev)
> return 0;
> err_clk:
> clk_unprepare(data->clk);
> +err_clk_sec:
> + if (!IS_ERR(data->clk_sec))
> + clk_unprepare(data->clk_sec);
> return ret;
> }
>
> @@ -715,6 +757,8 @@ static int exynos_tmu_remove(struct platform_device *pdev)
> exynos_unregister_thermal(data->reg_conf);
>
> clk_unprepare(data->clk);
> + if (!IS_ERR(data->clk_sec))
> + clk_unprepare(data->clk_sec);
>
> if (!IS_ERR(data->regulator))
> regulator_disable(data->regulator);
> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
> index 0d6b32f..60cce28 100644
> --- a/drivers/thermal/samsung/exynos_tmu.h
> +++ b/drivers/thermal/samsung/exynos_tmu.h
> @@ -43,6 +43,7 @@ enum soc_type {
> SOC_ARCH_EXYNOS4210 = 1,
> SOC_ARCH_EXYNOS4412,
> SOC_ARCH_EXYNOS5250,
> + SOC_ARCH_EXYNOS5420_TRIMINFO,

Here as well.

> SOC_ARCH_EXYNOS5440,
> };
>
[snip]
> +#define EXYNOS5420_TMU_DATA \
> + __EXYNOS5420_TMU_DATA \
> + .type = SOC_ARCH_EXYNOS5250, \
> + .features = (TMU_SUPPORT_EMULATION | TMU_SUPPORT_TRIM_RELOAD | \
> + TMU_SUPPORT_FALLING_TRIP | TMU_SUPPORT_READY_STATUS | \
> + TMU_SUPPORT_EMUL_TIME)
> +
> +#define EXYNOS5420_TMU_DATA_SHARED \
> + __EXYNOS5420_TMU_DATA \
> + .type = SOC_ARCH_EXYNOS5420_TRIMINFO, \
> + .features = (TMU_SUPPORT_EMULATION | TMU_SUPPORT_TRIM_RELOAD | \
> + TMU_SUPPORT_FALLING_TRIP | TMU_SUPPORT_READY_STATUS | \
> + TMU_SUPPORT_EMUL_TIME | TMU_SUPPORT_ADDRESS_MULTIPLE)
> +
> +struct exynos_tmu_init_data const exynos5420_default_tmu_data = {
> + .tmu_data = {
> + { EXYNOS5420_TMU_DATA },
> + { EXYNOS5420_TMU_DATA },
> + { EXYNOS5420_TMU_DATA_SHARED },
> + { EXYNOS5420_TMU_DATA_SHARED },
> + { EXYNOS5420_TMU_DATA_SHARED },

Shouldn't this be inferred from compatible string?

If I understand this code correctly, you can now always use
EXYNOS5420_TMU_DATA for 5420-tmu compatible string and
EXYNOS5420_TMU_DATA_SHARED for 5420-tmu-ext-triminfo.

Best regards,
Tomasz