From mboxrd@z Thu Jan 1 00:00:00 1970 From: jonghwa3.lee@samsung.com Subject: Re: [PATCH v2] Thermal: exynos: Add sysfs node supporting exynos's emulation mode. Date: Wed, 31 Oct 2012 16:44:55 +0900 Message-ID: <5090D6F7.5040401@samsung.com> References: <1351657147-26092-1-git-send-email-jonghwa3.lee@samsung.com> <4D68720C2E767A4AA6A8796D42C8EB591FDE36@BGSMSX101.gar.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mailout4.samsung.com ([203.254.224.34]:13232 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933553Ab2JaHpL (ORCPT ); Wed, 31 Oct 2012 03:45:11 -0400 In-reply-to: <4D68720C2E767A4AA6A8796D42C8EB591FDE36@BGSMSX101.gar.corp.intel.com> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: "R, Durgadoss" Cc: Jonghwa Lee , "linux-pm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "Brown, Len" , "Rafael J. Wysocki" , Amit Dinel Kachhap On 2012=EB=85=84 10=EC=9B=94 31=EC=9D=BC 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@samsung.com] >> Sent: Wednesday, October 31, 2012 9:49 AM >> To: linux-pm@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org; Brown, Len; R, Durgadoss; Rafael J= =2E >> Wysocki; Amit Dinel Kachhap; Jonghwa Lee >> Subject: [PATCH v2] Thermal: exynos: Add sysfs node supporting exyno= s'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 the= rmal >> 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 valu= e. >> This patch includes also documentary placed under >> Documentation/thermal/. >> >> Signed-off-by: Jonghwa Lee >> --- >> 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 >> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D >> + >> +Copyright (C) 2012 Samsung Electronics >> + >> +Writen by Jonghwa Lee >> + >> +Description >> +----------- >> + >> +Exynos 4x12 (4212, 4412) and 5 series provide emulation mode for th= ermal >> 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 f= rom >> 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 s= tate. When >> you input any >> +temperature you want to update to sysfs node, it automatically enab= le >> 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 r= eal >> sensing time, 938us.) >> + >> +Disabling emulation mode only requires writing value 0 to sysfs nod= e. >> + >> + >> +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 shee= t 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 =3D container_of(dev, >> + struct platform_device, dev); >> + struct exynos_tmu_data *data =3D platform_get_drvdata(pdev); >> + unsigned int reg; >> + u8 temp_code; >> + int temp, enable; >> + >> + mutex_lock(&data->lock); >> + clk_enable(data->clk); >> + reg =3D readl(data->base + EXYNOS_EMUL_CON); >> + clk_disable(data->clk); >> + mutex_unlock(&data->lock); >> + >> + enable =3D 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 >>=3D EXYNOS_EMUL_DATA_SHIFT; >> + temp_code =3D reg & EXYNOS_EMUL_DATA_MASK; >> + temp =3D code_to_temp(data, temp_code); >> + } else { >> + temp =3D 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 =3D container_of(dev, >> + struct platform_device, dev); >> + struct exynos_tmu_data *data =3D 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 =3D readl(data->base + EXYNOS_EMUL_CON); >> + enable =3D 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 a= lready is disabled, then emulation mode will be failed and we can't use it. Because emulati= on 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. Bu= t if you try to write 0 again to sysfs node, temp, then it write delay time with 938us and next tempe= rature with minimum temperature automatically. Value is changed but enable bit is still dis= abled. 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 =3D (reg >> EXYNOS_EMUL_DATA_SHIFT) & >> EXYNOS_EMUL_DATA_MASK; >> + temp_code =3D temp =3D=3D 0 ? temp_code : temp_to_code(data, temp)= ; >> + >> + reg =3D (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 th= at > 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 t= he case, temp =3D 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 conditio= nally 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 drive= r. 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" i= n > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >