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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ