| 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: Wed, 5 Jun 2013 08:53:00 -0400
From: Eduardo Valentin <eduardo.valentin@...com>
To: amit daniel kachhap <amit.daniel@...sung.com>
CC: Eduardo Valentin <eduardo.valentin@...com>,
<jonghwa3.lee@...sung.com>, <linux-pm@...r.kernel.org>,
Zhang Rui <rui.zhang@...el.com>,
<linux-samsung-soc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Kukjin Kim <kgene.kim@...sung.com>
Subject: Re: [PATCH V4 22/30] thermal: exynos: Add support for exynos5440
TMU sensor.
On 04-06-2013 23:20, amit daniel kachhap wrote:
> Hi Eduardo,
>
> On Tue, Jun 4, 2013 at 6:25 PM, Eduardo Valentin
> <eduardo.valentin@...com> wrote:
>> On 04-06-2013 00:44, amit daniel kachhap wrote:
>>> Hi Jonghwa,
>>>
>>> Sorry for the late reply as I was on leave.
>>>
>>> On Sat, May 18, 2013 at 10:53 AM, <jonghwa3.lee@...sung.com> wrote:
>>>> On 2013년 05월 14일 18:58, Amit Daniel Kachhap wrote:
>>>>
>>>>> This patch modifies TMU controller to add changes needed to work with
>>>>> exynos5440 platform. This sensor registers 3 instance of the tmu controller
>>>>> with the thermal zone and hence reports 3 temperature output. This controller
>>>>> supports upto five trip points. For critical threshold the driver uses the
>>>>> core driver thermal framework for shutdown.
>>>>>
>>>>> Acked-by: Kukjin Kim <kgene.kim@...sung.com>
>>>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@...sung.com>
>>>>> ---
>>>>> .../devicetree/bindings/thermal/exynos-thermal.txt | 28 ++++++++++++-
>>>>> drivers/thermal/samsung/exynos_tmu.c | 43 +++++++++++++++++--
>>>>> drivers/thermal/samsung/exynos_tmu.h | 6 +++
>>>>> drivers/thermal/samsung/exynos_tmu_data.h | 2 +
>>>>> 4 files changed, 72 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>>> index 535fd0e..970eeba 100644
>>>>> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>>> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>>> @@ -6,13 +6,16 @@
>>>>> "samsung,exynos4412-tmu"
>>>>> "samsung,exynos4210-tmu"
>>>>> "samsung,exynos5250-tmu"
>>>>> + "samsung,exynos5440-tmu"
>>>>> - interrupt-parent : The phandle for the interrupt controller
>>>>> -- reg : Address range of the thermal registers
>>>>> +- reg : Address range of the thermal registers. For exynos5440-tmu which has 3
>>>>> + instances of TMU, 2 set of register has to supplied. First set belongs
>>>>> + to each instance of TMU and second set belongs to common TMU registers.
>>>>> - interrupts : Should contain interrupt for thermal system
>>>>> - clocks : The main clock for TMU device
>>>>> - clock-names : Thermal system clock name
>>>>>
>>>>> -Example:
>>>>> +Example 1):
>>>>>
>>>>> tmu@...C0000 {
>>>>> compatible = "samsung,exynos4412-tmu";
>>>>> @@ -23,3 +26,24 @@ Example:
>>>>> clock-names = "tmu_apbif";
>>>>> status = "disabled";
>>>>> };
>>>>> +
>>>>> +Example 2):
>>>>> +
>>>>> + tmuctrl_0: tmuctrl@...118 {
>>>>> + compatible = "samsung,exynos5440-tmu";
>>>>> + reg = <0x160118 0x230>, <0x160368 0x10>;
>>>>> + interrupts = <0 58 0>;
>>>>> + clocks = <&clock 21>;
>>>>> + clock-names = "tmu_apbif";
>>>>> + };
>>>>> +
>>>>> +Note: For multi-instance tmu each instance should have an alias correctly
>>>>> +numbered in "aliases" node.
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +aliases {
>>>>> + tmuctrl0 = &tmuctrl_0;
>>>>> + tmuctrl1 = &tmuctrl_1;
>>>>> + tmuctrl2 = &tmuctrl_2;
>>>>> +};
>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>>>> index 7f7b1cf..7ca9c4d 100644
>>>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>>>> @@ -185,9 +185,11 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>> reg->threshold_th0 + i * sizeof(reg->threshold_th0));
>>>>>
>>>>> writel(reg->inten_rise_mask, data->base + reg->tmu_intclear);
>>>>> - } else if (data->soc == SOC_ARCH_EXYNOS) {
>>>>> + } else if (data->soc == SOC_ARCH_EXYNOS ||
>>>>> + data->soc == SOC_ARCH_EXYNOS5440) {
>>>>> /* Write temperature code for rising and falling threshold */
>>>>> - for (i = 0; i < trigger_levs; i++) {
>>>>> + for (i = 0;
>>>>> + i < trigger_levs && i < EXYNOS_MAX_TRIGGER_PER_REG; i++) {
>>>>> threshold_code = temp_to_code(data,
>>>>> pdata->trigger_levels[i]);
>>>>> if (threshold_code < 0) {
>>>>> @@ -218,7 +220,30 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>> writel((reg->inten_rise_mask << reg->inten_rise_shift) |
>>>>> (reg->inten_fall_mask << reg->inten_fall_shift),
>>>>> data->base + reg->tmu_intclear);
>>>>> +
>>>>> + /* if 5th threshold limit is also present, use TH2 register */
>>>>> + i = EXYNOS_MAX_TRIGGER_PER_REG;
>>>>> + if (pdata->trigger_levels[i]) {
>>>>> + threshold_code = temp_to_code(data,
>>>>> + pdata->trigger_levels[i]);
>>>>> + if (threshold_code < 0) {
>>>>> + ret = threshold_code;
>>>>> + goto out;
>>>>> + }
>>>>> + rising_threshold =
>>>>> + threshold_code << reg->threshold_th3_l0_shift;
>>>>> + writel(rising_threshold,
>>>>> + data->base + reg->threshold_th2);
>>>>> + if (pdata->trigger_type[i] == HW_TRIP) {
>>>>> + con = readl(data->base + reg->tmu_ctrl);
>>>>> + con |= (1 << reg->therm_trip_en_shift);
>>>>> + writel(con, data->base + reg->tmu_ctrl);
>>>>> + }
>>>>> + }
>>>>> }
>>>>> + /*Clear the PMIN in the common TMU register*/
>>>>> + if (reg->tmu_pmin && !data->id)
>>>>> + writel(0, data->base_common + reg->tmu_pmin);
>>>>> out:
>>>>> clk_disable(data->clk);
>>>>> mutex_unlock(&data->lock);
>>>>> @@ -345,7 +370,14 @@ static void exynos_tmu_work(struct work_struct *work)
>>>>> struct exynos_tmu_data, irq_work);
>>>>> struct exynos_tmu_platform_data *pdata = data->pdata;
>>>>> const struct exynos_tmu_registers *reg = pdata->registers;
>>>>> - unsigned int val_irq;
>>>>> + unsigned int val_irq, val_type;
>>>>> +
>>>>> + /* Find which sensor generated this interrupt */
>>>>> + if (reg->tmu_irqstatus) {
>>>>> + val_type = readl(data->base_common + reg->tmu_irqstatus);
>>>>> + if (!((val_type >> data->id) & 0x1))
>>>>> + goto out;
>>>>
>>>>
>>>> I have a question about your implementation for supporting EXYNOS5440.
>>>> I don't know exactly how EXYNO5440's tmu is working, but just guess it would be
>>>> similar with other EXYNOS series's without number of thermal sensors. (exclusive
>>>> register map and threshold level). Due to the multiple number of thermal sensor
>>>> in EXYNOS5440, it have multiple thermal zone devices and that's why it just
>>>> leave interrupt pin in pending if interrupt is not its, right?
>>> Yes in 5440 the interrupt line is shared so pending bit is left uncleared.
>>>>
>>>> So, my curious is, why we make all platform devices for each of thermal zone
>>>> devices? Why don't you just handle all thermal zone devices with one platform
>>>> device?
>>> Your doubt is genuine. Let me justify my design decision.
>>> Initially I also thought of making a single platform device but since
>>> there are 3 different TMU controllers and register maps for 4 more so
>>> I followed this design as the driver looks clean and can be scalable
>>> easily. Also I agree that some resources like IRQ line is shared but
>>> it is due to h/w limitation.
>>> Also it is easy to cleanly control each TMU instance with the device
>>> tree data similar to I2C/SPI/MMC instance based device driver. Say I
>>> do not want to use 2nd sensor then just pass device tree data for 1st
>>> and 3rd sensor.
>>>>
>>>> Yes, It's probably right to make multiple devices node to support them, because
>>>> it has different physical hardware(sensors). But we have one TMU , don't we?
>>>> (Maybe my assumption is wrong, I assume that it has one TMU because it looks
>>>> like it has only one irq line.). If I'm right, I think it is better to manage
>>>> all thermal zone devices with one platform device. Then, we don't need to leave
>>>> irq handler with leaving it pendded like above and also we may not need other
>>>> your patches like adding base_common iomem variable.
>>> Agreed that base_common variables is extra and present to handle the
>>> common part. I will further analyse your suggestion.
>>>
>>
>> What is the relation TMU <--> temperature sensor? Is it one to one or
>> one to many?
> 1 TMU --- > 1 temp sensor.(one to one)
>
OK. Then it is different to TI bandgap, which has one to many relation.
Does every TMU has its own resources, like a register map and IRQ?
> Thanks,
> Amit Daniel
>>
>>> Thanks,
>>> Amit Daniel
>>>>
>>>> I'd like to listen your opinion about this.
>>>>
>>>> Thanks,
>>>> Jonghwa
>>>>
>>>>> + }
>>>>>
>>>>> exynos_report_trigger(data->reg_conf);
>>>>> mutex_lock(&data->lock);
>>>>> @@ -358,7 +390,7 @@ static void exynos_tmu_work(struct work_struct *work)
>>>>>
>>>>> clk_disable(data->clk);
>>>>> mutex_unlock(&data->lock);
>>>>> -
>>>>> +out:
>>>>> enable_irq(data->irq);
>>>>> }
>>>>>
>>>>> @@ -520,7 +552,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>>>>> return ret;
>>>>>
>>>>> if (pdata->type == SOC_ARCH_EXYNOS ||
>>>>> - pdata->type == SOC_ARCH_EXYNOS4210)
>>>>> + pdata->type == SOC_ARCH_EXYNOS4210 ||
>>>>> + pdata->type == SOC_ARCH_EXYNOS5440)
>>>>> data->soc = pdata->type;
>>>>> else {
>>>>> ret = -EINVAL;
>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
>>>>> index 65443d7..9151a30 100644
>>>>> --- a/drivers/thermal/samsung/exynos_tmu.h
>>>>> +++ b/drivers/thermal/samsung/exynos_tmu.h
>>>>> @@ -44,6 +44,7 @@ enum trigger_type {
>>>>> enum soc_type {
>>>>> SOC_ARCH_EXYNOS4210 = 1,
>>>>> SOC_ARCH_EXYNOS,
>>>>> + SOC_ARCH_EXYNOS5440,
>>>>> };
>>>>>
>>>>> /**
>>>>> @@ -132,6 +133,8 @@ enum soc_type {
>>>>> * @emul_temp_shift: shift bits of emulation temperature.
>>>>> * @emul_time_shift: shift bits of emulation time.
>>>>> * @emul_time_mask: mask bits of emulation time.
>>>>> + * @tmu_irqstatus: register to find which TMU generated interrupts.
>>>>> + * @tmu_pmin: register to get/set the Pmin value.
>>>>> */
>>>>> struct exynos_tmu_registers {
>>>>> u32 triminfo_data;
>>>>> @@ -199,6 +202,9 @@ struct exynos_tmu_registers {
>>>>> u32 emul_temp_shift;
>>>>> u32 emul_time_shift;
>>>>> u32 emul_time_mask;
>>>>> +
>>>>> + u32 tmu_irqstatus;
>>>>> + u32 tmu_pmin;
>>>>> };
>>>>>
>>>>> /**
>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
>>>>> index 0e2244f..4acf070 100644
>>>>> --- a/drivers/thermal/samsung/exynos_tmu_data.h
>>>>> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
>>>>> @@ -91,6 +91,8 @@
>>>>> #define EXYNOS_EMUL_DATA_MASK 0xFF
>>>>> #define EXYNOS_EMUL_ENABLE 0x1
>>>>>
>>>>> +#define EXYNOS_MAX_TRIGGER_PER_REG 4
>>>>> +
>>>>> #if defined(CONFIG_CPU_EXYNOS4210)
>>>>> extern struct exynos_tmu_platform_data const exynos4210_default_tmu_data;
>>>>> #define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)
>>>>
>>>>
>>>
>>>
>>
>>
>> --
>> You have got to be excited about what you are doing. (L. Lamport)
>>
>> Eduardo Valentin
>>
>
>
--
You have got to be excited about what you are doing. (L. Lamport)
Eduardo Valentin
Download attachment "signature.asc" of type "application/pgp-signature" (296 bytes)
Powered by blists - more mailing lists