| 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: <1595080.7e4t1E1lOO@amdc1032>
Date: Mon, 25 Aug 2014 13:28:29 +0200
From: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
To: Chanwoo Choi <cw00.choi@...sung.com>
Cc: linux-arm-kernel@...ts.infradead.org, eduardo.valentin@...com,
amit.daniel@...sung.com, rui.zhang@...el.com, mark.rutland@....com,
devicetree@...r.kernel.org, kgene.kim@...sung.com,
pawel.moll@....com, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org,
Eduardo Valentin <edubezval@...il.com>,
linux-samsung-soc@...r.kernel.org, kyungmin.park@...sung.com,
robh+dt@...nel.org, galak@...eaurora.org, ch.naveen@...sung.com
Subject: Re: [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL
registers
On Monday, August 25, 2014 01:19:04 PM Bartlomiej Zolnierkiewicz wrote:
> On Monday, August 25, 2014 07:37:25 PM Chanwoo Choi wrote:
> > Hi Bartlomiej,
> >
> > On 08/25/2014 07:15 PM, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > Hi,
> > >
> > > On Monday, August 25, 2014 04:30:23 PM Chanwoo Choi wrote:
> > >> This patch support many TRIMINFO_CTRL registers if specific Exynos SoC
> > >> has one more TRIMINFO_CTRL registers. Also this patch uses proper 'RELOAD'
> > >> shift/mask bit operation to set RELOAD feature instead of static value.
> > >>
> > >> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
> > >> Acked-by: Kyungmin Park <kyungmin.park@...sung.com>
> > >> Cc: Zhang Rui <rui.zhang@...el.com>
> > >> Cc: Eduardo Valentin <edubezval@...il.com>
> > >> Cc: Amit Daniel Kachhap <amit.daniel@...sung.com>
> > >> Reviewed-by: Amit Daniel Kachhap <amit.daniel@...sung.com>
> > >> ---
> > >> drivers/thermal/samsung/exynos_thermal_common.h | 1 +
> > >> drivers/thermal/samsung/exynos_tmu.c | 23 ++++++++++++++++++++---
> > >> drivers/thermal/samsung/exynos_tmu.h | 9 +++++++--
> > >> drivers/thermal/samsung/exynos_tmu_data.c | 5 ++++-
> > >> drivers/thermal/samsung/exynos_tmu_data.h | 3 +++
> > >> 5 files changed, 35 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/drivers/thermal/samsung/exynos_thermal_common.h b/drivers/thermal/samsung/exynos_thermal_common.h
> > >> index 3eb2ed9..b211976 100644
> > >> --- a/drivers/thermal/samsung/exynos_thermal_common.h
> > >> +++ b/drivers/thermal/samsung/exynos_thermal_common.h
> > >> @@ -28,6 +28,7 @@
> > >> #define MAX_TRIP_COUNT 8
> > >> #define MAX_COOLING_DEVICE 4
> > >> #define MAX_THRESHOLD_LEVS 5
> > >> +#define MAX_TRIMINFO_CTRL_REG 2
> > >>
> > >> #define ACTIVE_INTERVAL 500
> > >> #define IDLE_INTERVAL 10000
> > >> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > >> index acbff14..7234f38 100644
> > >> --- a/drivers/thermal/samsung/exynos_tmu.c
> > >> +++ b/drivers/thermal/samsung/exynos_tmu.c
> > >> @@ -147,7 +147,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> > >> struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> > >> struct exynos_tmu_platform_data *pdata = data->pdata;
> > >> const struct exynos_tmu_registers *reg = pdata->registers;
> > >> - unsigned int status, trim_info = 0, con;
> > >> + unsigned int status, trim_info = 0, con, ctrl;
> > >> unsigned int rising_threshold = 0, falling_threshold = 0;
> > >> int ret = 0, threshold_code, i, trigger_levs = 0;
> > >>
> > >> @@ -164,8 +164,25 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> > >> }
> > >> }
> > >>
> > >> - if (TMU_SUPPORTS(pdata, TRIM_RELOAD))
> > >> - __raw_writel(1, data->base + reg->triminfo_ctrl);
> > >> + if (TMU_SUPPORTS(pdata, TRIM_RELOAD)) {
> > >> + if (reg->triminfo_ctrl_count > MAX_TRIMINFO_CTRL_REG) {
> > >
> > > Please remove this check and MAX_TRIMINFO_CTRL_REG define.
> > >
> > > We do not want such runtime checks for development time errors.
> >
> > OK, I'll remove it.
> >
> > >
> > >> + ret = -EINVAL;
> > >> + goto out;
> > >> + }
> > >> +
> > >> + for (i = 0; i < reg->triminfo_ctrl_count; i++) {
> > >> + if (pdata->triminfo_reload[i]) {
> > >> + ctrl = readl(data->base +
> > >> + reg->triminfo_ctrl[i]);
> > >> + ctrl &= ~(reg->triminfo_reload_mask <<
> > >> + reg->triminfo_reload_shift);
> > >> + ctrl |= pdata->triminfo_reload[i] <<
> > >> + reg->triminfo_reload_shift;
> > >
> > > triminfo_reload_shift and triminfo_reload_mask variables have always
> > > the same values when this code is run so there is no need for them.
> >
> > I don't understand. Do you mean that timinfo_reload_{shift/mask} variable is un-needed?
>
> Yes.
>
> > If you possible, I need more detailed comment.
>
> Currently triminfo_reload_shift is always "0" and triminfo_reload_mask
> is "1" so there is no need to add an abstraction for different SoCs
> (it should be added only when there is a real need for it).
>
> Please just rewrite this code as:
>
> if (pdata->triminfo_reload[i]) {
> ctrl = readl(data->base +
> reg->triminfo_ctrl[i]);
> ctrl |= pdata->triminfo_reload[i];
> __raw_writel(ctrl, data->base +
> reg->triminfo_ctrl[i]);
> }
or even better lets get rid of the only __raw_writel() call in Exynos
thermal driver while at it:
if (pdata->triminfo_reload[i]) {
ctrl = readl(data->base +
reg->triminfo_ctrl[i]);
ctrl |= pdata->triminfo_reload[i];
writel(ctrl, data->base +
reg->triminfo_ctrl[i]);
}
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
--
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