Messages in this thread Patch in this message | | | From | Balsam CHIHI <> | Date | Wed, 18 Jan 2023 14:58:46 +0100 | 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@collabora.com> wrote: > > Il 12/01/23 16:28, bchihi@baylibre.com ha scritto: > > From: Balsam CHIHI <bchihi@baylibre.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@baylibre.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@baylibre.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@baylibre.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
| |