| 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
| ||
|
Message-id: <5090D6F7.5040401@samsung.com>
Date: Wed, 31 Oct 2012 16:44:55 +0900
From: jonghwa3.lee@...sung.com
To: "R, Durgadoss" <durgadoss.r@...el.com>
Cc: Jonghwa Lee <jonghwa3.lee@...sung.com>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Brown, Len" <len.brown@...el.com>,
"Rafael J. Wysocki" <rjw@...k.pl>,
Amit Dinel Kachhap <amit.kachhap@...aro.org>
Subject: Re: [PATCH v2] Thermal: exynos: Add sysfs node supporting exynos's
emulation mode.
On 2012년 10월 31일 15:45, R, Durgadoss wrote:
> Hi,
>
> Looks like a nice feature :-)
> Without something like this, we had been spending time on
> writing test drivers, to actually test our thermal framework code.
Yes, fortunately, Exynos SOCs emulation mode makes our life better. ; )
> BTW, against which tree this patch was generated ?
> Rui's -next or master or Linux-next ?
>
> Some comments below, on a quick glance..
It is based on Rui's -next branch.
>> -----Original Message-----
>> From: Jonghwa Lee [mailto:jonghwa3.lee@...sung.com]
>> Sent: Wednesday, October 31, 2012 9:49 AM
>> To: linux-pm@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org; Brown, Len; R, Durgadoss; Rafael J.
>> Wysocki; Amit Dinel Kachhap; Jonghwa Lee
>> Subject: [PATCH v2] Thermal: exynos: Add sysfs node supporting exynos's
>> emulation mode.
>>
>> This patch supports exynos's emulation mode with newly created sysfs node.
>> Exynos 4x12 (4212, 4412) and 5 series provide emulation mode for thermal
>> management unit. Thermal emulation mode supports software debug for
>> TMU's
>> operation. User can set temperature manually with software code and TMU
>> will read current temperature from user value not from sensor's value.
>> This patch includes also documentary placed under
>> Documentation/thermal/.
>>
>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@...sung.com>
>> ---
>> v2
>> exynos_thermal.c
>> - Fix build error occured by wrong emulation control register name.
>> - Remove exynos5410 dependent codes.
>> exynos_theraml_emulation
>> - Align indentation.
>>
>> Documentation/thermal/exynos_thermal_emulation | 49 +++++++++++++
>> drivers/thermal/Kconfig | 9 +++
>> drivers/thermal/exynos_thermal.c | 88
>> ++++++++++++++++++++++++
>> 3 files changed, 146 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/thermal/exynos_thermal_emulation
>>
>> diff --git a/Documentation/thermal/exynos_thermal_emulation
>> b/Documentation/thermal/exynos_thermal_emulation
>> new file mode 100644
>> index 0000000..062d867
>> --- /dev/null
>> +++ b/Documentation/thermal/exynos_thermal_emulation
>> @@ -0,0 +1,49 @@
>> +EXYNOS EMULATION MODE
>> +========================
>> +
>> +Copyright (C) 2012 Samsung Electronics
>> +
>> +Writen by Jonghwa Lee <jonghwa3.lee@...sung.com>
>> +
>> +Description
>> +-----------
>> +
>> +Exynos 4x12 (4212, 4412) and 5 series provide emulation mode for thermal
>> management unit.
>> +Thermal emulation mode supports software debug for TMU's operation.
>> User can set temperature
>> +manually with software code and TMU will read current temperature from
>> user value not from
>> +sensor's value.
>> +
>> +Enabling CONFIG_EXYNOS_THERMAL_EMUL option will make this support
>> in available.
>> +When it's enabled, sysfs node will be created under
>> +/sys/bus/platform/devices/'exynos device name'/ with name of
>> 'emulation'.
>> +
>> +The sysfs node, 'emulation', will contain value 0 for the initial state. When
>> you input any
>> +temperature you want to update to sysfs node, it automatically enable
>> emulation mode and
>> +current temperature will be changed into it.
>> +(Exynos also supports user changable delay time which would be used to
>> delay of
>> + changing temperature. However, this node only uses same delay of real
>> sensing time, 938us.)
>> +
>> +Disabling emulation mode only requires writing value 0 to sysfs node.
>> +
>> +
>> +TEMP 120 |
>> + |
>> + 100 |
>> + |
>> + 80 |
>> + | +-----------
>> + 60 | | |
>> + | +-------------| |
>> + 40 | | | |
>> + | | | |
>> + 20 | | | +----------
>> + | | | | |
>> + 0
>> |______________|_____________|__________|__________|_______
>> __
>> + A A A A TIME
>> + |<----->| |<----->| |<----->| |
>> + | 938us | | | | | |
>> +emulation : 0 50 | 70 | 20 | 0
>> +current temp : sensor 50 70 20 sensor
> Thanks for the documentation. Is there a publicly available data sheet for this?
> If so, please provide the link here.
I'm sorry it isn't.
>> +
>> +
>> +
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index e1cb6bd..c02a66c 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -55,3 +55,12 @@ config EXYNOS_THERMAL
>> help
>> If you say yes here you get support for TMU (Thermal Managment
>> Unit) on SAMSUNG EXYNOS series of SoC.
>> +
>> +config EXYNOS_THERMAL_EMUL
>> + bool "EXYNOS TMU emulation mode support"
>> + depends on !CPU_EXYNOS4210 && EXYNOS_THERMAL
>> + help
>> + Exynos 4412 and 4414 and 5 series has emulation mode on TMU.
>> + Enable this option will be make sysfs node in exynos thermal
>> platform
>> + device directory to support emulation mode. With emulation mode
>> sysfs
>> + node, you can manually input temperature to TMU for simulation
>> purpose.
>> diff --git a/drivers/thermal/exynos_thermal.c
>> b/drivers/thermal/exynos_thermal.c
>> index fd03e85..9e3c150 100644
>> --- a/drivers/thermal/exynos_thermal.c
>> +++ b/drivers/thermal/exynos_thermal.c
>> @@ -99,6 +99,15 @@
>> #define IDLE_INTERVAL 10000
>> #define MCELSIUS 1000
>>
>> +#ifdef CONFIG_EXYNOS_THERMAL_EMUL
>> +#define EXYNOS_EMUL_TIME 0x57F0
>> +#define EXYNOS_EMUL_TIME_SHIFT 16
>> +#define EXYNOS_EMUL_DATA_SHIFT 8
>> +#define EXYNOS_EMUL_DATA_MASK 0xFF
>> +#define EXYNOS_EMUL_DISABLE 0x0
>> +#define EXYNOS_EMUL_ENABLE 0x1
>> +#endif /* CONFIG_EXYNOS_THERMAL_EMUL */
>> +
>> /* CPU Zone information */
>> #define PANIC_ZONE 4
>> #define WARN_ZONE 3
>> @@ -832,6 +841,82 @@ static inline struct exynos_tmu_platform_data
>> *exynos_get_driver_data(
>> return (struct exynos_tmu_platform_data *)
>> platform_get_device_id(pdev)->driver_data;
>> }
>> +
>> +#ifdef CONFIG_EXYNOS_THERMAL_EMUL
>> +static ssize_t exynos_tmu_emulation_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> + struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>> + unsigned int reg;
>> + u8 temp_code;
>> + int temp, enable;
>> +
>> + mutex_lock(&data->lock);
>> + clk_enable(data->clk);
>> + reg = readl(data->base + EXYNOS_EMUL_CON);
>> + clk_disable(data->clk);
>> + mutex_unlock(&data->lock);
>> +
>> + enable = reg & EXYNOS_EMUL_ENABLE;
> Looks like we don't need this variable 'enable'.
> Also, initialize temp to 0 in the beginning, which will save the
> else part of the code.
Some maintainers warn me off using 0 initializing at beginning.
Anyway, I'll follow you this time.
>> +
>> + if (enable) {
>> + reg >>= EXYNOS_EMUL_DATA_SHIFT;
>> + temp_code = reg & EXYNOS_EMUL_DATA_MASK;
>> + temp = code_to_temp(data, temp_code);
>> + } else {
>> + temp = 0;
>> + }
>> +
>> + return sprintf(buf, "%d\n", temp);
>> +}
>> +
>> +static ssize_t exynos_tmu_emulation_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> + struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>> + unsigned int reg;
>> + u8 temp_code;
>> + int temp, enable, i;
>> +
>> + if (!sscanf(buf, "%d\n", &temp))
>> + return -EINVAL;
>> +
>> + if (temp < 0)
>> + return -EINVAL;
> Why not combine both the if s using || ?
Okay, I'll fix it.
>> +
>> + mutex_lock(&data->lock);
>> + clk_enable(data->clk);
>> +
>> + reg = readl(data->base + EXYNOS_EMUL_CON);
>> + enable = reg & EXYNOS_EMUL_ENABLE;
>> + if (!enable && !temp)
>> + goto out;
> I think you what you are trying to do here is this:
> If the emulation is already disabled, and 'this' write tries to
> disable it again, you jump to 'out' as an optimization.
> Please correct me if I am wrong.
Yes, you're right. If we try to disable the emulation mode despite it already is disabled,
then emulation mode will be failed and we can't use it. Because emulation mode has some
strange characteristic.
Let me explain more detail. Emulation mode requires synchronous of any value changing and
enabling. It means you can't change any value (next target temperature, sensing time delay)
without enabling. In this code, disabling overlapping makes problem at initial state. At the initial,
there is no next target temperature and only value 1 for delay time. But if you try to write 0 again
to sysfs node, temp, then it write delay time with 938us and next temperature with minimum
temperature automatically. Value is changed but enable bit is still disabled. This is what the
problem is happened.
> In this case, name the variable is_enabled or something like
> that, which makes the check more explicit.
Okay, I'll consider of it.
>> +
>> + temp_code = (reg >> EXYNOS_EMUL_DATA_SHIFT) &
>> EXYNOS_EMUL_DATA_MASK;
>> + temp_code = temp == 0 ? temp_code : temp_to_code(data, temp);
>> +
>> + reg = (EXYNOS_EMUL_TIME << EXYNOS_EMUL_TIME_SHIFT) |
>> + (temp_code << EXYNOS_EMUL_DATA_SHIFT) |
>> + (temp > 0 ? EXYNOS_EMUL_ENABLE :
>> EXYNOS_EMUL_DISABLE);
> 2 things here:
> 1. Can we make the temp > 0 comparison as a separate statement, so that
> it is easy to read/maintain ?
>
> 2. you are comparing temp > 0 here, but above you return when temp < 0.
> Looks like, in this case, the flow will never reach here, when temp < 0.
Yes, The case, temp < 0 , will never get here. but I want to separate the case, temp = 0 and
temp > 0. I'll modify this to be more clear.
>> +
>> + writel(reg, data->base + EXYNOS_EMUL_CON);
>> +out:
>> + clk_disable(data->clk);
>> + mutex_unlock(&data->lock);
>> +
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR(emulation, 0644, exynos_tmu_emulation_show,
>> + exynos_tmu_emulation_store);
>> +#endif
>> +
>> static int __devinit exynos_tmu_probe(struct platform_device *pdev)
>> {
>> struct exynos_tmu_data *data;
>> @@ -930,6 +1015,9 @@ static int __devinit exynos_tmu_probe(struct
>> platform_device *pdev)
>> dev_err(&pdev->dev, "Failed to register thermal
>> interface\n");
>> goto err_clk;
>> }
>> +#ifdef CONFIG_EXYNOS_THERMAL_EMUL
>> + device_create_file(&pdev->dev, &dev_attr_emulation);
> Don't we want to capture the return value here ?
> Also, if we conditionally create this, we should remove this conditionally also.
> I did not see a corresponding device_remove_file for this attr.
Sorry, It's my mistake. I'll fix it.
>> +#endif
> Can we club all the code inside CONFIG_*_EMUL at one place ?
> This will make it neat, and not spread the #ifdefs all over the driver.
I'd like to do either. But I just couldn't find a way.
Could you give me any idea how to do it ?
Best regards,
Jonghwa Lee.
> Thanks,
> Durga
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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