Re: [PATCH 3/3 v8] thermal: samsung: Add TMU support for Exynos5420SoCs

From: Tomasz Figa
Date: Thu Nov 07 2013 - 10:10:10 EST

Next message: Naveen Krishna Ch: "Re: [PATCH] thermal: exynos: handle gate clock for misplaced TRIMINFO register"
Previous message: Arnaldo Carvalho de Melo: "[PATCH 1/7] perf evsel: Ditch evsel->handler.data field"
In reply to: Naveen Krishna Chatradhi: "[PATCH 3/3 v8] thermal: samsung: Add TMU support for Exynos5420 SoCs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Naveen,

On Thursday 07 of November 2013 11:23:32 Naveen Krishna Chatradhi wrote:
> This patch adds the neccessary register changes and arch information
> to support Exynos5420 SoCs
> Exynos5420 has 5 TMU channels one for each CPU 0, 1, 2 and 3 and GPU
>
> Also updated the Documentation at
> Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>
> Note: The platform data structure will be handled properly once the driver
> moves to complete device driver solution.

Huh? I'm not sure what do you mean here.

> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@xxxxxxxxxxx>
> ---
> Changes since v1:
> 1. modified the platform data structure in order to pass SHARED flag
> for channels that need sharing of address space.
> 2. https://lkml.org/lkml/2013/8/1/38 is merged into this patch.
> As the changes are minimum and can be added here.
> Changes since v3:
> a. Rearraged the code alphabetically, make exynso5420 come before exynso5440
> b. Reduce code duplication in passing platform data by introducing a common macro
> Bartlomiej Zolnierkiewicz Thanks for review and suggestions
> Changes since v4:
> None
> Changes since v5:
> None
> Changes since v6:
> - removed the unsued field "inten_fall_shift"
> - defined EXYNOS5420_TMU_CLEAR_FALL_INT_SHIFT instead of using EXYNOS_TMU_FALL_INT_SHIFT
> Changes since v7:
> - changes ins v6 were moved to the patch 1/3 of this patchset.
> - defined EXYNOS5420_TMU_CLEAR_FALL_INT_SHIFT instead of using EXYNOS_TMU_FALL_INT_SHIFT
>
> .../devicetree/bindings/thermal/exynos-thermal.txt | 39 ++++++++
> drivers/thermal/samsung/exynos_tmu.c | 12 ++-
> drivers/thermal/samsung/exynos_tmu.h | 1 +
> drivers/thermal/samsung/exynos_tmu_data.c | 98 ++++++++++++++++++++
> drivers/thermal/samsung/exynos_tmu_data.h | 8 ++
> 5 files changed, 157 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
> index 116cca0..c5f9a74 100644
> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
> @@ -6,6 +6,7 @@
> "samsung,exynos4412-tmu"
> "samsung,exynos4210-tmu"
> "samsung,exynos5250-tmu"
> + "samsung,exynos5420-tmu"

I would add a second compatible value here for TMU units that have
misplaced TRIMINFO data, e.g. "samsung,exynos5420-tmu-broken-triminfo"
and explicitly specify that second reg and clock-names entry is required
for this compatible value.

> "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 +14,16 @@
> interrupt related then 2 set of register has to supplied. First set
> belongs to each instance of TMU and second set belongs to second set
> of common TMU registers.

nit: A blank line here would be nice.

> + NOTE: On Exynos5420, 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
> +
> + The misplaced register address is passed through devicetree as the
> + second base
> +
> - interrupts : Should contain interrupt for thermal system
> - clocks : The main clock for TMU device
> - clock-names : Thermal system clock name
> @@ -43,6 +54,34 @@ Example 2):
> clock-names = "tmu_apbif";
> };
>
> +Example 3): (In case of Exynos5420)

Maybe "in case of misplaced TRIMINFO register" would be better?

> + /* tmu for CPU2 */
> + tmu@10068000 {
> + compatible = "samsung,exynos5420-tmu";
> + reg = <0x10068000 0x100>, <0x1006c000 0x4>;
> + interrupts = <0 184 0>;
> + clocks = <&clock 318>;
> + clock-names = "tmu_apbif";
> + };
> +

I believe that just a single example of a node for a TMU with misplaced
TRIMINFO register will be enough.

> + /* tmu for CPU3 */
> + tmu@1006c000 {
> + compatible = "samsung,exynos5420-tmu";
> + reg = <0x1006c000 0x100>, <0x100a0000 0x4>;
> + interrupts = <0 185 0>;
> + clocks = <&clock 318>;
> + clock-names = "tmu_apbif";
> + };
> +
> + /* tmu for GPU */
> + tmu@100a0000 {
> + compatible = "samsung,exynos5420-tmu";
> + reg = <0x100a0000 0x100>, <0x10068000 0x4>;
> + interrupts = <0 215 0>;
> + clocks = <&clock 318>;
> + clock-names = "tmu_apbif";
> + };
> +
> Note: For multi-instance tmu each instance should have an alias correctly
> numbered in "aliases" node.
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index ae80a87..b54825a 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -186,7 +186,12 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> EXYNOS5440_EFUSE_SWAP_OFFSET + reg->triminfo_data);
> }
> } else {
> - trim_info = readl(data->base + reg->triminfo_data);
> + /* On exynos5420 the triminfo register is in the shared space */
> + if (data->base_second && (data->soc == SOC_ARCH_EXYNOS5420))

This is ugly. What about having a quirk based description, that would
allow to have code like this (just an example, not ready code):

if (data->quirks & EXYNOS_TMU_MISPLACED_TRIMINFO)

> + trim_info = readl(data->base_second +
> + reg->triminfo_data);
> + else
> + trim_info = readl(data->base + reg->triminfo_data);
> }
> data->temp_error1 = trim_info & EXYNOS_TMU_TEMP_MASK;
> data->temp_error2 = ((trim_info >> reg->triminfo_85_shift) &
> @@ -498,6 +503,10 @@ static const struct of_device_id exynos_tmu_match[] = {
[snip]
> +#define EXYNOS5420_TMU_DATA \
> + __EXYNOS5420_TMU_DATA \
> + .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 \
> + .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 },
> + },
> + .tmu_count = 5,
> +};

Is this, by any chance, matching by some kind of block index? If yes, this
is awfully broken, when all of them are separate IP blocks.

What if an SoC shows up with particular TMU channels compatible with
Exynos 5420, but ordered differently? (e.g. GPU, CPU0, CPU2, CPU1, CPU3)

Instead, such data as contained in exynos_tmu_init_data should be rather
determined by IP compatible value, just as I suggested earlier in this
post.

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/

Next message: Naveen Krishna Ch: "Re: [PATCH] thermal: exynos: handle gate clock for misplaced TRIMINFO register"
Previous message: Arnaldo Carvalho de Melo: "[PATCH 1/7] perf evsel: Ditch evsel->handler.data field"
In reply to: Naveen Krishna Chatradhi: "[PATCH 3/3 v8] thermal: samsung: Add TMU support for Exynos5420 SoCs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]