Message ID | 20250430123306.15072-1-linux.amoon@gmail.com |
---|---|
Headers | show |
Series | Exynos Thermal code improvement | expand |
Hi All, On Wed, 30 Apr 2025 at 18:03, Anand Moon <linux.amoon@gmail.com> wrote: > > Hi All, > > This patch series is a rework of my previous patch series [1], > where the code changes were not adequately justified. > > In this new series, I have improved the commit subject > and commit message to better explain the changes. > > v6: Add new patch to use devm_clk_get_enabled > and Fix few typo in subject as suggested by Daniel. > v5: Drop the guard mutex patch > v4: Tried to address Lukasz review comments. > > Tested on Odroid U3 amd XU4 SoC boards. > Build with clang with W=1 enable. > Genital Ping!!! Thanks -Anand
On 08/05/2025 08:14, Anand Moon wrote: > Hi All, > > On Wed, 30 Apr 2025 at 18:03, Anand Moon <linux.amoon@gmail.com> wrote: >> >> Hi All, >> >> This patch series is a rework of my previous patch series [1], >> where the code changes were not adequately justified. >> >> In this new series, I have improved the commit subject >> and commit message to better explain the changes. >> >> v6: Add new patch to use devm_clk_get_enabled >> and Fix few typo in subject as suggested by Daniel. >> v5: Drop the guard mutex patch >> v4: Tried to address Lukasz review comments. >> >> Tested on Odroid U3 amd XU4 SoC boards. >> Build with clang with W=1 enable. >> > > Genital Ping!!! Huhu, nice. :) I make typos as well, but some typos are better to avoid. :) Anyway, !!! are exclamation marks and I think it is very difficult to scream at someone gently. I think this is contradictory to itself, so it does not feel gently at all. Plus you sent it 7 days ago and you are known to send poor quality, untested code, so just relax and wait. Best regards, Krzysztof
Hi Krzysztof, On Thu, 8 May 2025 at 11:57, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 08/05/2025 08:14, Anand Moon wrote: > > Hi All, > > > > On Wed, 30 Apr 2025 at 18:03, Anand Moon <linux.amoon@gmail.com> wrote: > >> > >> Hi All, > >> > >> This patch series is a rework of my previous patch series [1], > >> where the code changes were not adequately justified. > >> > >> In this new series, I have improved the commit subject > >> and commit message to better explain the changes. > >> > >> v6: Add new patch to use devm_clk_get_enabled > >> and Fix few typo in subject as suggested by Daniel. > >> v5: Drop the guard mutex patch > >> v4: Tried to address Lukasz review comments. > >> > >> Tested on Odroid U3 amd XU4 SoC boards. > >> Build with clang with W=1 enable. > >> > > > > Genital Ping!!! > > > Huhu, nice. :) > I make typos as well, but some typos are better to avoid. :) > > Anyway, !!! are exclamation marks and I think it is very difficult to > scream at someone gently. I think this is contradictory to itself, so it > does not feel gently at all. > I did not mean anything harsh with these !!! marks. > Plus you sent it 7 days ago and you are known to send poor quality, > untested code, so just relax and wait. > NAK - Do not merge these changes. The original code is in better condition, and the proposed modifications introduce issues. > Best regards, > Krzysztof Thanks -Anand
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. > This patch series is a rework of my previous patch series [1], > where the code changes were not adequately justified. > > In this new series, I have improved the commit subject > and commit message to better explain the changes. > > v6: Add new patch to use devm_clk_get_enabled > and Fix few typo in subject as suggested by Daniel. > v5: Drop the guard mutex patch > v4: Tried to address Lukasz review comments. > > Tested on Odroid U3 amd XU4 SoC boards. > Build with clang with W=1 enable. > > [4] https://lore.kernel.org/all/20250410063754.5483-2-linux.amoon@gmail.com/ > [3] https://lore.kernel.org/all/20250310143450.8276-2-linux.amoon@gmail.com/ > [2] https://lore.kernel.org/all/20250216195850.5352-2-linux.amoon@gmail.com/ > [1] https://lore.kernel.org/all/20220515064126.1424-1-linux.amoon@gmail.com/ > [0] https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#m77e57120d230d57f34c29e1422d7fc5f5587ac30 > > Thanks > -Anand > > Anand Moon (4): > thermal/drivers/exynos: Refactor clk_sec initialization inside > SOC-specific case > thermal/drivers/exynos: Use devm_clk_get_enabled() helpers > thermal/drivers/exynos: Remove redundant IS_ERR() checks for clk_sec > clock > thermal/drivers/exynos: Fixed the efuse min max value for exynos5422 > > drivers/thermal/samsung/exynos_tmu.c | 100 ++++++++++----------------- > 1 file changed, 35 insertions(+), 65 deletions(-) > > > base-commit: b6ea1680d0ac0e45157a819c41b46565f4616186 > -- > 2.49.0 >
Hi Daniel, On Wed, 14 May 2025 at 16:53, Daniel Lezcano <daniel.lezcano@linaro.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@100c0000 exynos5420 tmu_cpu0: tmu@10060000 tmu_cpu1: tmu@10064000 tmu_cpu2: tmu@10068000 tmu_cpu3: tmu@1006c000 tmu_gpu: tmu@100a0000 exynos5433 tmu_atlas0: tmu@10060000 tmu_atlas1: tmu@10068000 tmu_g3d: tmu@10070000 exynos7 tmu@10060000 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. Thanks -Anand
On 5/15/25 13:10, Anand Moon wrote: > Hi Daniel, > > On Wed, 14 May 2025 at 16:53, Daniel Lezcano <daniel.lezcano@linaro.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@100c0000 > exynos5420 > tmu_cpu0: tmu@10060000 > tmu_cpu1: tmu@10064000 > tmu_cpu2: tmu@10068000 > tmu_cpu3: tmu@1006c000 > tmu_gpu: tmu@100a0000 > exynos5433 > tmu_atlas0: tmu@10060000 > tmu_atlas1: tmu@10068000 > tmu_g3d: tmu@10070000 > exynos7 > tmu@10060000 > > 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
Hi Daniel, On Thu, 15 May 2025 at 18:59, Daniel Lezcano <daniel.lezcano@linaro.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@linaro.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@100c0000 > > exynos5420 > > tmu_cpu0: tmu@10060000 > > tmu_cpu1: tmu@10064000 > > tmu_cpu2: tmu@10068000 > > tmu_cpu3: tmu@1006c000 > > tmu_gpu: tmu@100a0000 > > exynos5433 > > tmu_atlas0: tmu@10060000 > > tmu_atlas1: tmu@10068000 > > tmu_g3d: tmu@10070000 > > exynos7 > > tmu@10060000 > > > > 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. Thank you for having a look into my changes 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
On 5/15/25 20:01, Anand Moon wrote: > Hi Daniel, > > On Thu, 15 May 2025 at 18:59, Daniel Lezcano <daniel.lezcano@linaro.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@linaro.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@100c0000 >>> exynos5420 >>> tmu_cpu0: tmu@10060000 >>> tmu_cpu1: tmu@10064000 >>> tmu_cpu2: tmu@10068000 >>> tmu_cpu3: tmu@1006c000 >>> tmu_gpu: tmu@100a0000 >>> exynos5433 >>> tmu_atlas0: tmu@10060000 >>> tmu_atlas1: tmu@10068000 >>> tmu_g3d: tmu@10070000 >>> exynos7 >>> tmu@10060000 >>> >>> 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; } But if the irq handler has some soc specific code, then it should be a separate function
Hi Daniel, On Fri, 16 May 2025 at 20:15, Daniel Lezcano <daniel.lezcano@linaro.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@linaro.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@linaro.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@100c0000 > >>> exynos5420 > >>> tmu_cpu0: tmu@10060000 > >>> tmu_cpu1: tmu@10064000 > >>> tmu_cpu2: tmu@10068000 > >>> tmu_cpu3: tmu@1006c000 > >>> tmu_gpu: tmu@100a0000 > >>> exynos5433 > >>> tmu_atlas0: tmu@10060000 > >>> tmu_atlas1: tmu@10068000 > >>> tmu_g3d: tmu@10070000 > >>> exynos7 > >>> tmu@10060000 > >>> > >>> 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