| 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: <CAGuA+ooxTf-j957gQ1zRe2-+u2kphaaLGvTDi1=kit5Q3bKOxA@mail.gmail.com>
Date: Wed, 18 Jan 2023 14:58:46 +0100
From: Balsam CHIHI <bchihi@...libre.com>
To: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>
Cc: daniel.lezcano@...aro.org, rafael@...nel.org, amitk@...nel.org,
rui.zhang@...el.com, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, khilman@...libre.com,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, james.lo@...iatek.com,
rex-bc.chen@...iatek.com
Subject: Re: [PATCH v10 4/6] thermal/drivers/mediatek: Add the Low Voltage
Thermal Sensor driver
Hi Angelo,
On Mon, Jan 16, 2023 at 11:50 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com> wrote:
>
> Il 12/01/23 16:28, bchihi@...libre.com ha scritto:
> > From: Balsam CHIHI <bchihi@...libre.com>
> >
> > The Low Voltage Thermal Sensor (LVTS) is a multiple sensors, multi
> > controllers contained in a thermal domain.
> >
> > A thermal domains can be the MCU or the AP.
> >
> > Each thermal domains contain up to seven controllers, each thermal
> > controller handle up to four thermal sensors.
> >
> > The LVTS has two Finite State Machines (FSM), one to handle the
> > functionin temperatures range like hot or cold temperature and another
> > one to handle monitoring trip point. The FSM notifies via interrupts
> > when a trip point is crossed.
> >
> > The interrupt is managed at the thermal controller level, so when an
> > interrupt occurs, the driver has to find out which sensor triggered
> > such an interrupt.
> >
> > The sampling of the thermal can be filtered or immediate. For the
> > former, the LVTS measures several points and applies a low pass
> > filter.
> >
> > Signed-off-by: Balsam CHIHI <bchihi@...libre.com>
> > ---
> > drivers/thermal/mediatek/Kconfig | 15 +
> > drivers/thermal/mediatek/Makefile | 1 +
> > drivers/thermal/mediatek/lvts_thermal.c | 1244 +++++++++++++++++++
> > include/dt-bindings/thermal/mediatek-lvts.h | 19 +
> > 4 files changed, 1279 insertions(+)
> > create mode 100644 drivers/thermal/mediatek/lvts_thermal.c
> > create mode 100644 include/dt-bindings/thermal/mediatek-lvts.h
> >
..snip..
> > +
> > +static irqreturn_t lvts_ctrl_irq_handler(struct lvts_ctrl *lvts_ctrl)
> > +{
> > + irqreturn_t iret = IRQ_NONE;
> > + u32 value, masks[] = { 0x0009001F, 0X000881F0, 0x00247C00, 0x1FC00000 };
>
> Please, no magic numbers around.
>
These number are explained in the comment bellow.
This is part of it :
* sensor 3 interrupt: 0001 1111 1100 0000 0000 0000 0000 0000
* => 0x1FC00000
* sensor 2 interrupt: 0000 0000 0010 0100 0111 1100 0000 0000
* => 0x00247C00
* sensor 1 interrupt: 0000 0000 0001 0001 0000 0011 1110 0000
* => 0X000881F0
* sensor 0 interrupt: 0000 0000 0000 1001 0000 0000 0001 1111
* => 0x0009001F
> > + int i;
> > +
> > + /*
> > + * Interrupt monitoring status
> > + *
> > + * LVTS_MONINTST
> > + *
> > + * Bits:
>
> You're describing the register with nice words, but there's another way to do
> the same that will be even more effective.
>
> /*
> * LVTS MONINT: Interrupt Monitoring register
> * Each bit describes the enable status of per-sensor interrupts.
> */
> #define LVTS_MONINT_THRES_COLD BIT(0) /* Cold threshold */
> #define LVTS_MONINT_THRES_HOT BIT(1) /* Hot threshold */
> #define LVTS_MONINT_OFFST_LOW BIT(2) /* Low offset */
> #define LVTS_MONINT_OFFST_HIGH BIT(3) /* High offset */
> #define LVTS_MONINT_OFFST_NTH BIT(4) /* Normal To Hot */
> #define EVERYTHING_ELSE ........................
>
> #define LVTS_MONINT_SNS0_MASK GENMASK( ... )
> #define LVTS_MONINT_SNS1_MASK GENMASK .....
>
> /* Find a better name for this one */
> #define LVTS_MONINT_EN_IRQS ( LVTS_MONINT_THRES_COLD | LVTS_MONINT_THRES_HOT |
> LVTS_MONINT_OFFST_LOW ..... etc etc)
>
Given the complexity of the controller and the number of registers,
if we create a define per bits, we will end up with a huge list of
defines (~300).
I don't think that will help for the readability.
> > + *
> > + * 31 : Interrupt for stage 3
> > + * 30 : Interrupt for stage 2
> > + * 29 : Interrupt for state 1
> > + * 28 : Interrupt using filter on sensor 3
> > + *
..snip..
> > + *
> > + * 3 : Interrupt high offset interrupt on sensor 0
> > + * 2 : Interrupt low offset interrupt on sensor 0
> > + * 1 : Interrupt hot threshold on sensor 0
> > + * 0 : Interrupt cold threshold on sensor 0
> > + *
> > + * We are interested in the sensor(s) responsible of the
> > + * interrupt event. We update the thermal framework with the
> > + * thermal zone associated with the sensor. The framework will
> > + * take care of the rest whatever the kind of interrupt, we
> > + * are only interested in which sensor raised the interrupt.
> > + *
> > + * sensor 3 interrupt: 0001 1111 1100 0000 0000 0000 0000 0000
> > + * => 0x1FC00000
> > + * sensor 2 interrupt: 0000 0000 0010 0100 0111 1100 0000 0000
> > + * => 0x00247C00
> > + * sensor 1 interrupt: 0000 0000 0001 0001 0000 0011 1110 0000
> > + * => 0X000881F0
> > + * sensor 0 interrupt: 0000 0000 0000 1001 0000 0000 0001 1111
> > + * => 0x0009001F
> > + */
> > + value = readl(LVTS_MONINTSTS(lvts_ctrl->base));
> > +
> > + /*
> > + * Let's figure out which sensors raised the interrupt
> > + *
> > + * NOTE: the masks array must be ordered with the index
> > + * corresponding to the sensor id eg. index=0, mask for
> > + * sensor0.
> > + */
> > + for (i = 0; i < ARRAY_SIZE(masks); i++) {
> > +
> > + if (!(value & masks[i]))
>
> Questions for you:
>
> 1. Are the masks always the same for all SoCs?
The LVTS controller is not SoC specific.
The mask is controller specific whatever the SoC version.
> 2. Do they correspond to what we set in lvts_irq_init()?
Not exactly, we set LVTS_MONINT and the controller sets the LVTS_MONINTSTS.
The content will be different with what we set and what we get.
>
> I'd expect future new SoCs to have different masks... and since lvts_irq_init() is
> actually "playing with" interrupts register(s), with one of them (LVTS_MONINT)
> having the same layout as this one, I would place all of the initialization in
> that function instead.
>
> This means that we'd initialize those masks at lvts_irq_init() time, in a struct
> member, and read it back in this interrupt handler: like that, we get that a bit
> more ordered and generally more readable.
>
No. Actually, what will change is on which sensor a thermal zone is tie to,
and that is handled already by the device tree configuration.
> > + continue;
> > +
> > + thermal_zone_device_update(lvts_ctrl->sensors[i].tz,
> > + THERMAL_TRIP_VIOLATED);
> > + iret |= IRQ_HANDLED;
> > + }
> > +
> > + /*
> > + * Write back to clear the interrupt status (W1C)
> > + */
> > + writel(value, LVTS_MONINTSTS(lvts_ctrl->base));
> > +
> > + return iret;
> > +}
> > +
> > +/*
> > + * Temperature interrupt handler. Even if the driver supports more
> > + * interrupt modes, we use the interrupt when the temperature crosses
> > + * the hot threshold the way up and the way down (modulo the
> > + * hysteresis).
> > + *
> > + * Each thermal domain has a couple of interrupts, one for hardware
> > + * reset and another one for all the thermal events happening on the
> > + * different sensors.
> > + *
> > + * The interrupt is configured for thermal events when crossing the
> > + * hot temperature limit. At each interrupt, we check in every
> > + * controller if there is an interrupt pending.
> > + */
> > +static irqreturn_t lvts_irq_handler(int irq, void *data)
> > +{
> > + struct lvts_domain *lvts_td = data;
> > + irqreturn_t iret = IRQ_NONE;
> > + int i;
> > +
> > + for (i = 0; i < lvts_td->num_lvts_ctrl; i++)
> > + iret |= lvts_ctrl_irq_handler(lvts_td->lvts_ctrl);
>
> Please do *not* OR your function calls! While this is surely fine here in
> this function and for this particular case, it's generally bad practice
> and shall be avoided.
>
I understand that could be prone to errors.
I can propose this alternative but it looks less elegant than OR'ing the result.
Do you have a suggestion to improve this code snippet?
> > +
> > + return iret;
> > +}
> > +
> > +static struct thermal_zone_device_ops lvts_ops = {
> > + .get_temp = lvts_get_temp,
> > + .set_trips = lvts_set_trips,
> > +};
> > +
>
> ..snip..
>
> > +
> > +static int lvts_irq_init(struct lvts_ctrl *lvts_ctrl)
> > +{
> > + u32 value;
> > +
> > + /*
> > + * LVTS_PROTCTL : Thermal Protection Sensor Selection
> > + *
> > + * Bits:
> > + *
> > + * 19-18 : Sensor to base the protection on
> > + * 17-16 : Strategy:
> > + * 00 : Average of 4 sensors
> > + * 01 : Max of 4 sensors
> > + * 10 : Selected sensor with bits 19-18
> > + * 11 : Reserved
> > + */
> > + value = BIT(16);
> > + writel(value, LVTS_PROTCTL(lvts_ctrl->base));
> > +
> > + /*
> > + * LVTS_PROTTA : Stage 1 temperature threshold
> > + * LVTS_PROTTB : Stage 2 temperature threshold
> > + * LVTS_PROTTC : Stage 3 temperature threshold
> > + *
> > + * Bits:
> > + *
> > + * 14-0: Raw temperature threshold
> > + *
> > + * writel(0x0, LVTS_PROTTA(lvts_ctrl->base));
> > + * writel(0x0, LVTS_PROTTB(lvts_ctrl->base));
> > + */
> > + writel(lvts_ctrl->hw_tshut_raw_temp, LVTS_PROTTC(lvts_ctrl->base));
> > +
> > + /*
> > + * LVTS_MONINT : Interrupt configuration register
> > + *
> > + * The LVTS_MONINT register layout is the same as the LVTS_MONINTSTS
> > + * register, except we set the bits to enable the interrupt.
> > + */
> > + value = 0x9FBF7BDE;
>
> u32 val;
>
> val = FIELD_PREP(LVTS_MONINT_SNS0_MASK, LVTS_MONINT_EN_IRQS);
> val |= FIELD_PREP(LVTS_MONINT_SNS1_MASK, LVTS_MONINT_EN_IRQS);
>
> ... etc
>
> writel(val, ...... )
>
OK, I'll change it accordingly.
> > + writel(value, LVTS_MONINT(lvts_ctrl->base));
> > +
> > + return 0;
> > +}
> > +
>
> ..snip..
>
> > +
> > +static int lvts_ctrl_initialize(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > + /*
> > + * Write device mask: 0xC1030000
> > + */
> > + u32 cmds[] = {
> > + 0xC1030E01, 0xC1030CFC, 0xC1030A8C, 0xC103098D, 0xC10308F1,
> > + 0xC10307A6, 0xC10306B8, 0xC1030500, 0xC1030420, 0xC1030300,
> > + 0xC1030030, 0xC10300F6, 0xC1030050, 0xC1030060, 0xC10300AC,
> > + 0xC10300FC, 0xC103009D, 0xC10300F1, 0xC10300E1
> > + };
> ...what is this long list of commands?
>
> Why 0xC103_0000? Describe that please.
>
AFAIU, based on the documentation, the configuration register can be
read or write.
When we write it, we set the different bits corresponding to a write sequence
which is 0xC1030000.
The documentation gives the register layout but does not explain how it works.
> Also, why is this not a platform data constant?
>
It is not a platform data, it is a controller data.
Whatever the SoC the configuration sequence will be the same.
> Example:
> struct lvts_plat {
> const struct lvts_ctrl_data *ctrl_data;
> u8 num_ctrl_data;
> const u16 device_mask;
> const u16 *init_cmds;
> u8 num_init_cmds;
> }
>
> where device_mask gets set to 0xc103 and init_cmds is an array containing
> the low-16 (0x0e01, 0x0cfc, ...), and where this function would simply do
> something like
>
> lvts_write_config(lvts_ctrl, plat->device_mask, init_cmds, num_init_cmds);
>
> ... and where lvts_write_config() does something like:
>
> for (i = 0; i < num_cmds; i++) {
> u32 val = device_mask | init_cmds[i];
> writel(val, LVTS_CONFIG ...)
> }
> > +
> > + lvts_write_config(lvts_ctrl, cmds, ARRAY_SIZE(cmds));
> > +
> > + return 0;
> > +}
> > +
> > +static int lvts_ctrl_calibrate(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > + int i;
> > + void __iomem *lvts_edata[] = {
>
> Can we constify this?
>
Constifying "void __iomem *lvts_edata[]" generates the following
compilation warning :
drivers/thermal/mediatek/lvts_thermal.c:835:47: warning: passing
argument 2 of ‘writel’ discards ‘const’ qualifier from pointer target
type [-Wdiscarded-qualifiers]
835 | writel(lvts_ctrl->calibration[i], lvts_edata[i]);
| ~~~~~~~~~~^~~
> > + LVTS_EDATA00(lvts_ctrl->base),
> > + LVTS_EDATA01(lvts_ctrl->base),
> > + LVTS_EDATA02(lvts_ctrl->base),
> > + LVTS_EDATA03(lvts_ctrl->base)
> > + };
> > +
> > + /*
> > + * LVTS_EDATA0X : Efuse calibration reference value for sensor X
> > + *
> > + * Bits:
> > + *
> > + * 20-0 : Efuse value for normalization data
> > + */
> > + for (i = 0; i < LVTS_SENSOR_MAX; i++)
> > + writel(lvts_ctrl->calibration[i], lvts_edata[i]);
> > +
> > + return 0;
> > +}
> > +
> > +static int lvts_ctrl_configure(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > + u32 period_unit = (118 * 1000) / (256 * 38);
>
> #define SOMETHING 118
> #define SOMETHING_ELSE 1000
> #define ....
>
> const u32 period_unit = (SOMETHING * SOMETHING_ELSE) / ....
>
Constifying "u32 period_unit" generates the following compilation warning :
./include/asm-generic/io.h:273:61: note: expected ‘volatile void *’
but argument is of type ‘const void *’
273 | static inline void writel(u32 value, volatile void __iomem *addr)
| ~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > + u32 grp_interval = 1;
> > + u32 flt_interval = 1;
> > + u32 sensor_interval = 1;
> > + u32 hw_filter = 0x2;
> > + u32 value;
> > +
>
> ...snip...
>
> > +
> > +static struct lvts_ctrl_data mt819x_lvts_data_ctrl[] = {
>
> No wildcards. Please, rename this to give the name of the oldest SoC
> that uses these values. Assuming that it is MT8192.... mt8192_lvts_data_ctrl[]
>
OK, it Will be mt8195_lvts_data_ctrl[].
> > + {
> > + .cal_offset = { 0x4, 0x7 },
> > + .lvts_sensor = {
> > + { .dt_id = MT819x_MCU_BIG_CPU0 },
> > + { .dt_id = MT819x_MCU_BIG_CPU1 }
> > + },
> > + .num_lvts_sensor = 2,
> > + .offset = 0x0,
> > + .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
> > + },
> > +
..snip..
> > +static struct lvts_data mt819x_lvts_mcu_data = {
>
> Same here.
>
OK, it Will be mt8195_lvts_mcu_data.
> > + .lvts_ctrl = mt819x_lvts_data_ctrl,
> > + .num_lvts_ctrl = ARRAY_SIZE(mt819x_lvts_data_ctrl),
> > +};
> > +
> > +static const struct of_device_id lvts_of_match[] = {
> > + { .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt819x_lvts_mcu_data },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, lvts_of_match);
> > +
> > +static struct platform_driver lvts_driver = {
> > + .probe = lvts_probe,
> > + .remove = lvts_remove,
> > + .driver = {
> > + .name = "mtk-lvts-thermal",
> > + .of_match_table = lvts_of_match,
> > + },
> > +};
> > +module_platform_driver(lvts_driver);
> > +
> > +MODULE_AUTHOR("Balsam CHIHI <bchihi@...libre.com>");
> > +MODULE_DESCRIPTION("MediaTek LVTS Thermal Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/dt-bindings/thermal/mediatek-lvts.h b/include/dt-bindings/thermal/mediatek-lvts.h
> > new file mode 100644
> > index 000000000000..80d060400236
> > --- /dev/null
> > +++ b/include/dt-bindings/thermal/mediatek-lvts.h
>
> Bindings go in a different commit: add this in your patch [2/6], where you are
> adding the yaml binding.
>
OK, it will be moved to :
[2/6] dt-bindings/thermal/mediatek: Add dt-binding document for LVTS
thermal controllers
> Also, please follow binding names: rename this file to mediatek,mt8192-lvts.h.
>
LVTS is SoC independent (only available on MT8192 and MT8195).
Should not we leave this file name SoC indemendent too "mediatek-lvts.h",
just like "mediatek,lvts-thermal.yaml"?
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2023 MediaTek Inc.
> > + * Author: Balsam CHIHI <bchihi@...libre.com>
> > + */
> > +
> > +#ifndef __MEDIATEK_LVTS_DT_H
> > +#define __MEDIATEK_LVTS_DT_H
> > +
> > +#define MT819x_MCU_BIG_CPU0 0
>
> No wildcards: MT8192_MCU_BIG_CPU0
>
OK, it Will be MT8195_MCU_BIG_CPU0.
>
> > +#define MT819x_MCU_BIG_CPU1 1
> > +#define MT819x_MCU_BIG_CPU2 2
> > +#define MT819x_MCU_BIG_CPU3 3
> > +#define MT819x_MCU_LITTLE_CPU0 4
> > +#define MT819x_MCU_LITTLE_CPU1 5
> > +#define MT819x_MCU_LITTLE_CPU2 6
> > +#define MT819x_MCU_LITTLE_CPU3 7
> > +
> > +#endif /* __MEDIATEK_LVTS_DT_H */
>
> Regards,
> Angelo
Best regards,
Balsam
Powered by blists - more mailing lists