| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 12 Nov 2013 11:49:34 +0530
From: Naveen Krishna Ch <naveenkrishna.ch@...il.com>
To: Tomasz Figa <t.figa@...sung.com>
Cc: Naveen Krishna Chatradhi <ch.naveen@...sung.com>,
linux-pm@...r.kernel.org, rui.zhang@...el.com,
eduardo.valentin@...com,
"linux-samsung-soc@...r.kernel.org"
<linux-samsung-soc@...r.kernel.org>, linux-kernel@...r.kernel.org,
amit.daniel@...sung.com, Kukjin Kim <kgene.kim@...sung.com>,
devicetree@...r.kernel.org, b.zolnierkie@...sung.com,
cpgs@...sung.com
Subject: Re: [PATCH 3/3 v8] thermal: samsung: Add TMU support for Exynos5420 SoCs
Hello Tomasz,
On 7 November 2013 20:39, Tomasz Figa <t.figa@...sung.com> wrote:
> 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.
This driver is handling platform data from a predefined structs in driver files.
Platform data is better handled when sent via Device tree nodes.
Will take up the device tree migration once this set is done.
>
>> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@...sung.com>
>> ---
>> 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.
Sure
>
>> "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?
Sure
>
>> + /* tmu for CPU2 */
>> + tmu@...68000 {
>> + 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.
right
>
>> + /* tmu for CPU3 */
>> + tmu@...6c000 {
>> + compatible = "samsung,exynos5420-tmu";
>> + reg = <0x1006c000 0x100>, <0x100a0000 0x4>;
>> + interrupts = <0 185 0>;
>> + clocks = <&clock 318>;
>> + clock-names = "tmu_apbif";
>> + };
>> +
>> + /* tmu for GPU */
>> + tmu@...a0000 {
>> + 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):
Right now the driverdata is a struct containing the register bases and
various operation
values for TMU, along with the soc specific details.
Will implement quirks, along with proper device tree migration of platform data
>
> 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.
I guess not.
>
> 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.
Right, will handling this along with device tree migration of platform data
>
> Best regards,
> Tomasz
>
--
Shine bright,
(: Nav :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists