Hello Tomasz,
On 18 December 2013 21:20, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote:
> 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.
Sure, will add them.
>
>> "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".
My bad, will correct this
>
>> + };
>> +
> [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.
My bad, will correct this
>
>> + 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 ||
TMU channel 0, 1 on Exynos5420 does not require a new pdata->type.
The are exactly similar to Exynos5250. Only the channels 2, 3 and 4
have the triminfo register misplaced.
Hence, just SOC_ARCH_EXYNOS5420_TRIMINFO should be enough
>
> 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.
Correct,
To avoid changes to exynos_get_driver_data() function the
exynos_get_driver_data struct is defined as above.
As function exynos_get_driver_data(struct platform_device *pdev, int id)
uses the id and matches the approriate structure pointer.
>
> Best regards,
> Tomasz
Thanks for your comments, will respin.
>
--
Shine bright,
(: Nav :)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
