| 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: <CANAwSgRnRPb9kPT1rxHFUEvyzoCLTrD5JZVtLHZ9A6gV00dOCw@mail.gmail.com>
Date: Sun, 18 May 2025 01:11:07 +0530
From: Anand Moon <linux.amoon@...il.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>, Krzysztof Kozlowski <krzk@...nel.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, Zhang Rui <rui.zhang@...el.com>,
Lukasz Luba <lukasz.luba@....com>, Alim Akhtar <alim.akhtar@...sung.com>,
Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
"open list:SAMSUNG THERMAL DRIVER" <linux-pm@...r.kernel.org>,
"open list:SAMSUNG THERMAL DRIVER" <linux-samsung-soc@...r.kernel.org>,
"moderated list:ARM/SAMSUNG S3C, S5P AND EXYNOS ARM ARCHITECTURES" <linux-arm-kernel@...ts.infradead.org>,
open list <linux-kernel@...r.kernel.org>,
"open list:CLANG/LLVM BUILD SUPPORT:Keyword:b(?i:clang|llvm)b" <llvm@...ts.linux.dev>
Subject: Re: [PATCH v6 0/4] Exynos Thermal code improvement
Hi Daniel,
On Fri, 16 May 2025 at 20:15, Daniel Lezcano <daniel.lezcano@...aro.org> wrote:
>
> On 5/15/25 20:01, Anand Moon wrote:
> > Hi Daniel,
> >
> > On Thu, 15 May 2025 at 18:59, Daniel Lezcano <daniel.lezcano@...aro.org> wrote:
> >>
> >> On 5/15/25 13:10, Anand Moon wrote:
> >>> Hi Daniel,
> >>>
> >>> On Wed, 14 May 2025 at 16:53, Daniel Lezcano <daniel.lezcano@...aro.org> wrote:
> >>>>
> >>>> On Wed, Apr 30, 2025 at 06:02:56PM +0530, Anand Moon wrote:
> >>>>> Hi All,
> >>>>
> >>>> Hi Anand,
> >>>>
> >>>> if the goal of the changes is to do cleanups, I recommend to rework
> >>>> how the code is organized. Instead of having the data->soc check all
> >>>> around the functions, write per platform functions and store them in
> >>>> struct of_device_id data field instead of the soc version.
> >>>>
> >>>> Basically get rid of exynos_map_dt_data by settings the different ops
> >>>> in a per platform structure.
> >>>>
> >>>> Then the initialization routine would be simpler to clean.
> >>>>
> >>>
> >>> Thanks, I had previously attempted this approach.
> >>> The goal is to split the exynos_tmu_data structure to accommodate
> >>> SoC-specific callbacks for initialization and configuration.
> >>>
> >>> In my earlier attempt, I tried to refactor the code to achieve this.
> >>> However, the main challenge I encountered was that the
> >>> exynos_sensor_ops weren’t being correctly mapped for each SoC.
> >>>
> >>> Some SoC have multiple sensor
> >>> exynos4x12
> >>> tmu: tmu@...c0000
> >>> exynos5420
> >>> tmu_cpu0: tmu@...60000
> >>> tmu_cpu1: tmu@...64000
> >>> tmu_cpu2: tmu@...68000
> >>> tmu_cpu3: tmu@...6c000
> >>> tmu_gpu: tmu@...a0000
> >>> exynos5433
> >>> tmu_atlas0: tmu@...60000
> >>> tmu_atlas1: tmu@...68000
> >>> tmu_g3d: tmu@...70000
> >>> exynos7
> >>> tmu@...60000
> >>>
> >>> It could be a design issue of the structure.or some DTS issue.
> >>> So what I found in debugging it is not working correctly.
> >>>
> >>> static const struct thermal_zone_device_ops exynos_sensor_ops = {
> >>> .get_temp = exynos_get_temp,
> >>> .set_emul_temp = exynos_tmu_set_emulation,
> >>> .set_trips = exynos_set_trips,
> >>> };
> >>>
> >>> The sensor callback will not return a valid pointer and soc id for the get_temp.
> >>>
> >>> Here is my earlier version of local changes.
> >>> [1] https://pastebin.com/bbEP04Zh exynos_tmu.c
> >>> [2] https://pastebin.com/PzNz5yve Odroid U3 dmesg.log
> >>> [3] https://pastebin.com/4Yjt2d2u Odroid Xu4 dmesg.log
> >>>
> >>> I want to re-model the structure to improve the code.
> >>> Once Its working condition I will send this for review.
> >>>
> >>> If you have some suggestions please let me know.
> >>
> >> I suggest to do the conversion step by step beginning by
> >> exynos4210_tmu_clear_irqs, then by exynos_map_dt_data as the first
> >> cleanup iteration
> >>
> > Ok you want IRQ handle per SoC call back functions?
> > so on all the changes depending on SoC id should be moved to
> > respective callback functions to reduce the code.
>
> I think you can keep the same irq handler function but move the
> tmu_intstat, tmu_intclear in the persoc structure and remove the
> exynos4210_tmu_clear_irqs function.
>
> You should end up with something like:
>
> static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
> {
> struct exynos_tmu_data *data = id;
> unsigned int val_irq;
>
> thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
>
> mutex_lock(&data->lock);
> clk_enable(data->clk);
>
> val_irq = readl(data->base + data->tmu_intstat);
> writel(val_irq, data->base + data->tmu_intclear);
>
> clk_disable(data->clk);
> mutex_unlock(&data->lock);
>
> return IRQ_HANDLED;
> }
No this will not work,
>
> But if the irq handler has some soc specific code, then it should be a
> separate function
>
INTSTAT interrupt status register holds the pending status of
rising and falling edge of IRQ
#define INTSTAT_FALL2 BIT(24)
#define INTSTAT_FALL1 BIT(20)
#define INTSTAT_FALL0 BIT(16)
#define INTSTAT_RISE2 BIT(8)
#define INTSTAT_RISE1 BIT(4)
#define INTSTAT_RISE0 BIT(0)
#define INTCLEAR_FALL2 BIT(24)
#define INTCLEAR_FALL1 BIT(20)
#define INTCLEAR_FALL0 BIT(16)
#define INTCLEAR_RISE2 BIT(8)
#define INTCLEAR_RISE1 BIT(4)
#define INTCLEAR_RISE0 BIT(0)
static void exynos4210_tmu_clear_irqs(struct exynos_tmu_data *data)
{
u32 tmu_intstat, tmu_intclear;
u32 val_irq = 0, clear_mask = 0;
if (data->soc == SOC_ARCH_EXYNOS5260) {
tmu_intstat = EXYNOS5260_TMU_REG_INTSTAT;
tmu_intclear = EXYNOS5260_TMU_REG_INTCLEAR;
} else if (data->soc == SOC_ARCH_EXYNOS7) {
tmu_intstat = EXYNOS7_TMU_REG_INTPEND;
tmu_intclear = EXYNOS7_TMU_REG_INTPEND;
} else if (data->soc == SOC_ARCH_EXYNOS5433) {
tmu_intstat = EXYNOS5433_TMU_REG_INTPEND;
tmu_intclear = EXYNOS5433_TMU_REG_INTPEND;
} else {
tmu_intstat = EXYNOS_TMU_REG_INTSTAT;
tmu_intclear = EXYNOS_TMU_REG_INTCLEAR;
}
val_irq = readl(data->base + tmu_intstat);
/*
* Clear the interrupts. Please note that the documentation for
* Exynos3250, Exynos4412, Exynos5250 and Exynos5260 incorrectly
* states that INTCLEAR register has a different placing of bits
* responsible for FALL IRQs than INTSTAT register. Exynos5420
* and Exynos5440 documentation is correct (Exynos4210 doesn't
* support FALL IRQs at all).
*/
/* Map INTSTAT bits to INTCLEAR bits */
if (val_irq & BIT(24))
clear_mask |= BIT(24);
else if (val_irq & BIT(20))
clear_mask |= BIT(20);
else if (val_irq & BIT(16))
clear_mask |= BIT(16);
else if (val_irq & BIT(8))
clear_mask |= BIT(8);
else if (val_irq & BIT(4))
clear_mask |= BIT(4);
else if (val_irq & BIT(0))
clear_mask |= BIT(0);
/* Perform proper task for decrease temperature */
if (clear_mask)
writel(clear_mask, data->base + tmu_intclear);
}
TMU clears the rising and falling interrupt according to the user manual.
Thanks
-Anand
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
Powered by blists - more mailing lists