Re: [PATCH 2/2] thermal: exynos: Add support for ARTPEC-8

[Krzysztof Kozlowski <krzk@kernel.org>]

On 22/03/2022 09:22, Sang Min Kim wrote:
> Add support thermal management for Axis ARTPEC-8 SoC.
> ARTPEC-8 is the SoC platform of Axis Communications.
> In the existing thermal management function of exynos, functions that support
> remote sensors have been added.
>  
> Signed-off-by: sangmin kim <hypmean.kim@samsung.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 666 ++++++++++++++++++++++++++++++++---
>  1 file changed, 616 insertions(+), 50 deletions(-)
>  
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index f4ab4c5..9837f42 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -14,6 +14,7 @@
>  #include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/interrupt.h>
> +#include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/of_address.h>
> @@ -124,6 +125,77 @@
>  
>  #define MCELSIUS        1000
>  
> +/* Artpec8 specific registers */
Pu all defines just after Exynos specific bits, so before MCELSIUS.

> +#define ARTPEC8_TMU_REG_TRIMINFO        0x0
> +#define ARTPEC8_TMU_REG_TRIMINFO1        0x4
> +#define ARTPEC8_TMU_REG_TRIMINFO2        0x8
> +#define ARTPEC8_TMU_REG_TRIMINFO3        0xC
> +#define ARTPEC8_TMU_REG_TRIMINFO4        0x10
> +#define ARTPEC8_TMU_REG_TRIMINFO5        0x14
> +#define ARTPEC8_TMU_REG_CONTROL                0x20
> +#define ARTPEC8_TMU_REG_CONTROL1        0x24
> +#define ARTPEC8_TMU_REG_STATUS                0x28
> +
> +#define ARTPEC8_TMU_REG_AVG_CONTROL        0x38
> +#define ARTPEC8_TMU_REG_TMU_TRIM0        0x3C
> +
> +#define ARTPEC8_TMU_REG_EMUL_CON        0x160
> +#define NUM_PROBE_OFFSET                16

This is not prefixed with artpec and is very cryptic. Please document
this and other defines which are not obvious, comparing to current code.

> +
> +#define ARTPEC8_FIRST_POINT_TRIM        25
> +#define ARTPEC8_SECOND_POINT_TRIM        105
> +
> +#define ARTPEC8_EMUL_EN                        1
> +#define ARTPEC8_TIME_OFFSET                16

Don't duplicate defines with existing ones. Define only differences.
This applies to all other defines as well.

> +#define ARTPEC8_EMUL_NEXT_TIME                (0x4e20 << ARTPEC8_TIME_OFFSET)
> +
> +#define ARTPEC8_TMU_TEMP_MASK                0x1ff
> +#define ARTPEC8_CALIB_SEL_SHIFT                23
> +
> +#define ARTPEC8_EMUL_DATA_SHIFT                7
> +
> +#define ARTPEC8_T_BUF_VREF_SEL_SHIFT        18
> +#define ARTPEC8_T_BUF_SLOPE_SEL_SHIFT        18
> +#define ARTPEC8_INTEN_TRIPPING_SHIFT        7
> +#define ARTPEC8_INTEN_CLOCKDOWN_SHIFT        8
> +#define ARTPEC8_TRIMINFO_105_SHIFT        9
> +#define ARTPEC8_INTEN_FALL0_SHIFT        16
> +#define ARTPEC8_TMU_REF_VOLTAGE_SHIFT        24
> +#define ARTPEC8_TMU_REF_VOLTAGE_MASK        0x1f
> +#define ARTPEC8_TMU_BUF_SLOPE_SEL_SHIFT        8
> +#define ARTPEC8_TMU_BUF_SLOPE_SEL_MASK        0xf
> +
> +#define ARTPEC8_TMU_CONTROL_CORE_EN        1
> +#define ARTPEC8_TMU_CONTROL_AUTO_MODE        2
> +#define ARTPEC8_TMU_CONTROL_TRIP_EN        (1 << 12)
> +#define ARTPEC8_LPI_MODE_EN                (1 << 10)
> +
> +#define ARTPEC8_TRIM0_BGR_I_SHIFT        20
> +#define ARTPEC8_TRIM0_VREF_SHIFT        12
> +#define ARTPEC8_TRIM0_VBE_I_SHIFT        8
> +
> +#define INTPEND_RISE_MASK                0xff
> +#define INTPEND_FALL_MASK                0xff0000

prefix

> +#define ARTPEC8_TRIM0_MASK                0xf
> +#define ARTPEC8_TRIM2_MASK                0x7
> +
> +#define ARTPEC8_TRIMINFO_TRIM0_SHIFT        18
> +
> +#define LOW_TEMP_WEIGHT                        9203
> +#define HIGH_TEMP_WEIGHT                9745
> +#define TEMP_WEIGHT                        10000

All these need explanation why/what/how did you get these values.

> +
> +struct sensor_offset {

kerneldoc needed

> +        u32 trim_offset;
> +        u32 temp_offset;
> +        u32 temp_reg_shift;
> +        u32 rise_offset;
> +        u32 fall_offset;
> +        u32 past_offset;
> +        u32 inten;
> +        u32 intpend;
> +};
> +
>  enum soc_type {
>          SOC_ARCH_EXYNOS3250 = 1,
>          SOC_ARCH_EXYNOS4210,
> @@ -134,6 +206,63 @@ enum soc_type {
>          SOC_ARCH_EXYNOS5420_TRIMINFO,
>          SOC_ARCH_EXYNOS5433,
>          SOC_ARCH_EXYNOS7,
> +        SOC_ARCH_ARTPEC8,

Put it alphabetically.

> +};
> +
> +#define SENSOR(_tr, _te, _sh, _ri, _fa, _pa, _en, _pend)        \
> +        {                                        \
> +                .trim_offset        = _tr,                \
> +                .temp_offset        = _te,                \
> +                .temp_reg_shift        = _sh,                \
> +                .rise_offset        = _ri,                \
> +                .fall_offset        = _fa,                \
> +                .past_offset        = _pa,                \
> +                .inten                = _en,                \
> +                .intpend        = _pend,                \
> +        }
> +
> +static const struct sensor_offset artpec8_sensors[] = {
> +        SENSOR(0x0,        0x40,        0,  0x50,        0x60,        0x70,        0x110,        0x118),

0x118 is existing value, right?
All these should be using rather a macro - either dedicated defines or
offset-based macros.

> +        SENSOR(0x4,        0x40,        9,  0x170,        0x180,        0x90,        0x120,        0x128),

Here and further it looks like you have all registers distributed
according to specific pattern. Define macro, probably with an offset
based on first sensor.

> +        SENSOR(0x8,        0x44,        0,  0x190,        0x1a0,        0xb0,        0x130,        0x138),
> +        SENSOR(0xc,        0x44,        9,  0x1b0,        0x1c0,        0xd0,        0x140,        0x148),
> +        SENSOR(0x10,        0x44,        18, 0x1d0,        0x1e0,        0xf0,        0x150,        0x158),
> +        SENSOR(0x14,        0x48,        0,  0x1f0,        0x200,        0x250,        0x310,        0x318),
> +};
> +
> +/**
> + * struct artpec8_sensor: A structure to hold the private data of the sensor
> + * @tmudev: The tmu device which this sensor is connected.
> + * @tzd: Thermal zonde device pointer to register this sensor.
> + * @id: Identifier of the one instance of the thermal sensor.
> + * @ntrip: Number of threshols for this sensor.
> + * @triminfo_25: OTP information to trim temperature sensor error for 25C
> + * @triminfo_105: OTP information to trim temperature sensor error for 105C
> + * @trim_offset: Offset of triminfo register.
> + * @temp_offset: Offset of current temperature. The temperature values of
> + *                2 to 3 remote sensors are stored in this register.
> + * @temp_reg_shift: start location of each tempt in temp_off
> + * @rise_offset: Offset of rising threshold level 6 and 7.
> + * @fall_offset: Offset of falling thershold level 6 and 7.
> + * @past_offset: Offset of Past temperature 0,1.
> + * @inten: Offset of interrupt enable sfr.
> + * @intpend: Offset of interrupt pending sfr.
> + */
> +struct artpec8_sensor {
> +        struct exynos_tmu_data *tmudev;
> +        struct thermal_zone_device *tzd;

Why does the sensor duplicate struct exynos_tmu_data?

> +        int id;
> +        unsigned int ntrip;
> +        u16 triminfo_25;
> +        u16 triminfo_105;
> +        u32 trim_offset;
> +        u32 temp_offset;
> +        u32 temp_reg_shift;
> +        u32 rise_offset;
> +        u32 fall_offset;
> +        u32 past_offset;
> +        u32 inten;
> +        u32 intpend;

You have all these in sensor_offset, don't you? Why do you need them
second time?

>  };
>  
>  /**
> @@ -193,6 +322,7 @@ struct exynos_tmu_data {
>          struct thermal_zone_device *tzd;
>          unsigned int ntrip;
>          bool enabled;
> +        u32 nr_remote;

Missing doc. Did you compile your code with W=1?

>  
>          void (*tmu_set_trip_temp)(struct exynos_tmu_data *data, int trip,
>                                   u8 temp);
> @@ -203,6 +333,8 @@ struct exynos_tmu_data {
>          int (*tmu_read)(struct exynos_tmu_data *data);
>          void (*tmu_set_emulation)(struct exynos_tmu_data *data, int temp);
>          void (*tmu_clear_irqs)(struct exynos_tmu_data *data);
> +
> +        struct artpec8_sensor sensor[0];

No artpec8_sensor, but Exynos sensor. You need to convert existing code
for working with 1 sensors and X sensors. Don't just add X sensors
duplicating parts of driver.

Also - why this is array of [0]?

>  };
>  
>  /*
> @@ -220,6 +352,28 @@ static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
>                  data->temp_error1;
>  }
>  
> +static u16 artpec8_temp_to_code(struct artpec8_sensor *sensor, int temp)

Maintain consistent code with existing implementation. Why types are
different than temp_to_code()? Maybe temp_to_code() is not good?

> +{
> +        int code;
> +        int weight;
> +
> +        if (sensor->tmudev->cal_type == TYPE_ONE_POINT_TRIMMING)
> +                return temp + sensor->triminfo_25 - ARTPEC8_FIRST_POINT_TRIM;
> +
> +        if (temp > ARTPEC8_FIRST_POINT_TRIM)
> +                weight = HIGH_TEMP_WEIGHT;
> +        else
> +                weight = LOW_TEMP_WEIGHT;
> +
> +        code = DIV_ROUND_CLOSEST((temp - ARTPEC8_FIRST_POINT_TRIM) *
> +                (sensor->triminfo_105 - sensor->triminfo_25) * TEMP_WEIGHT,
> +                (ARTPEC8_SECOND_POINT_TRIM - ARTPEC8_FIRST_POINT_TRIM) *
> +                weight);
> +        code += sensor->triminfo_25;
> +
> +        return (u16)code;
> +}
> +
>  /*
>   * Calculate a temperature value from a temperature code.
>   * The unit of the temperature is degree Celsius.
> @@ -235,6 +389,27 @@ static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
>                  EXYNOS_FIRST_POINT_TRIM;
>  }
>  
> +static int artpec8_code_to_temp(struct artpec8_sensor *sensor, u16 code)
> +{
> +        int temp;
> +        int weight;
> +
> +        if (sensor->tmudev->cal_type == TYPE_ONE_POINT_TRIMMING)
> +                return code - sensor->triminfo_25 + ARTPEC8_FIRST_POINT_TRIM;
> +
> +        if (code > sensor->triminfo_25)
> +                weight = HIGH_TEMP_WEIGHT;
> +        else
> +                weight = LOW_TEMP_WEIGHT;
> +
> +        temp = DIV_ROUND_CLOSEST((code - sensor->triminfo_25) *
> +                (ARTPEC8_SECOND_POINT_TRIM - ARTPEC8_FIRST_POINT_TRIM) * weight,
> +                (sensor->triminfo_105 - sensor->triminfo_25) * TEMP_WEIGHT);
> +        temp += ARTPEC8_FIRST_POINT_TRIM;
> +
> +        return temp;
> +}
> +
>  static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
>  {
>          u16 tmu_temp_mask =
> @@ -338,7 +513,8 @@ static u32 get_con_reg(struct exynos_tmu_data *data, u32 con)
>          con &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
>          con |= data->reference_voltage << EXYNOS_TMU_REF_VOLTAGE_SHIFT;
>  
> -        con &= ~(EXYNOS_TMU_BUF_SLOPE_SEL_MASK << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> +        con &= ~(EXYNOS_TMU_BUF_SLOPE_SEL_MASK <<
> +                        EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);

How is this related?

>          con |= (data->gain << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
>  
>          con &= ~(EXYNOS_TMU_TRIP_MODE_MASK << EXYNOS_TMU_TRIP_MODE_SHIFT);
> @@ -558,6 +734,120 @@ static void exynos7_tmu_initialize(struct platform_device *pdev)
>          sanitize_temp_error(data, trim_info);
>  }
>  
> +static void artpec8_tmu_set_trip_temp(struct exynos_tmu_data *data,
> +                int trip, int temp, int remote)
> +{
> +        unsigned int reg_off, bit_off;
> +        u32 th;
> +        struct artpec8_sensor *sensor;
> +        unsigned int temp_rise;
> +
> +        sensor = &data->sensor[remote];
> +        temp_rise = sensor->rise_offset;
> +
> +        reg_off = ((7 - trip) / 2) * 4;
> +        bit_off = ((8 - trip) % 2);
Please explain the offsets in comment.

> +
> +        th = readl(data->base + temp_rise + reg_off);
> +        th &= ~(ARTPEC8_TMU_TEMP_MASK << (16 * bit_off));
> +        th |= artpec8_temp_to_code(sensor, temp) << (16 * bit_off);
> +        writel(th, data->base + temp_rise + reg_off);
> +}
> +
> +static void artpec8_tmu_set_trip_hyst(struct exynos_tmu_data *data,
> +                int trip, int temp, int hyst, int remote)
> +{
> +        unsigned int reg_off, bit_off;
> +        u32 th;
> +        struct artpec8_sensor *sensor;
> +        unsigned int temp_fall;
> +
> +        sensor = &data->sensor[remote];
> +        temp_fall = sensor->fall_offset;
> +
> +        reg_off = ((7 - trip) / 2) * 4;
> +        bit_off = ((8 - trip) % 2);
> +
> +        th = readl(data->base + temp_fall + reg_off);
> +        th &= ~(ARTPEC8_TMU_TEMP_MASK << (16 * bit_off));
> +        th |= artpec8_temp_to_code(sensor, temp - hyst) << (16 * bit_off);
> +        writel(th, data->base + temp_fall + reg_off);
> +}
> +
> +static void artpec8_tmu_clear_irqs(struct exynos_tmu_data *data, int i)
> +{
> +        u32 intp = readl(data->base + data->sensor[i].intpend);
> +
> +        writel(intp, data->base + data->sensor[i].intpend);
> +}
> +

I'll skip reviewing this part. Half of it will be gone once you convert
driver to have uniform approach to sensors.

(...)

> +static int artpec8_register_tzd(struct platform_device *pdev)
> +{
> +        struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> +        struct artpec8_sensor *sensor;
> +        struct device *dev = &pdev->dev;
> +        int sensor_idx, ret = 0;
> +        struct thermal_zone_device *tzd;
> +        const struct thermal_trip *trips;
> +
> +        for (sensor_idx = 0; sensor_idx < data->nr_remote; sensor_idx++) {
> +                sensor = &data->sensor[sensor_idx];
> +
> +                ret = artpec8_map_sensor_data(data, sensor_idx, sensor);
> +                if (ret)
> +                        break;
> +
> +                tzd = devm_thermal_zone_of_sensor_register(dev,
> +                                sensor_idx, sensor, &artpec8_ops);
> +                if (IS_ERR(tzd))
> +                        continue;
> +
> +                sensor->tzd = tzd;
> +                trips = of_thermal_get_trip_points(tzd);
> +                if (!trips) {
> +                        dev_warn(dev,
> +                                "Cannot get trip points from device tree!\n");
> +                        ret = -ENODEV;
> +                        break;
> +                }
> +                sensor->ntrip = of_thermal_get_ntrips(tzd);
> +        }
> +
> +        return ret;
> +}
> +
>  static int exynos_tmu_probe(struct platform_device *pdev)
>  {
>          struct exynos_tmu_data *data;
>          int ret;
> +        int sensor_idx;
> +        int nr_remote = 0;
> +        struct device *dev;
> +        const struct of_device_id *dev_id;
>  
> -        data = devm_kzalloc(&pdev->dev, sizeof(struct exynos_tmu_data),
> -                                        GFP_KERNEL);
> +        if (pdev->dev.of_node)
> +                dev = &pdev->dev;
> +        else
> +                dev = pdev->dev.parent;

Whaaaaat?

> +
> +        dev_id = of_match_node(exynos_tmu_match, dev->of_node);
> +        if (dev_id) {
> +              �� data = (struct exynos_tmu_data *)dev_id->data;

Some unusual characters appeared here.

Did you run checkpatch, smatch and sparse?

> +        } else {
> +                dev_warn(dev, "dev id error\n");

Is it possible?

> +                return -EINVAL;
> +        }
> +
> +        ret = of_property_read_u32(dev->of_node, "remote_sensors", &nr_remote);

Do not add undocumented properties.

> +        if (ret < 0)
> +                data = devm_kzalloc(&pdev->dev, sizeof(struct exynos_tmu_data),
> +                                    GFP_KERNEL);
> +        else
> +                data = devm_kzalloc(dev, sizeof(struct exynos_tmu_data) +
> +                        (sizeof(struct artpec8_sensor) * nr_remote),
> +                        GFP_KERNEL);
>          if (!data)
>                  return -ENOMEM;
>  
>          platform_set_drvdata(pdev, data);
>          mutex_init(&data->lock);
>  
> +        if (data->soc != SOC_ARCH_ARTPEC8) {
>          /*
>           * Try enabling the regulator if found
>           * TODO: Add regulator as an SOC feature, so that regulator enable
>           * is a compulsory call.
>           */

Does not look like proper indentation.

> -        data->regulator = devm_regulator_get_optional(&pdev->dev, "vtmu");
> -        if (!IS_ERR(data->regulator)) {
> -                ret = regulator_enable(data->regulator);
> -                if (ret) {
> -                        dev_err(&pdev->dev, "failed to enable vtmu\n");
> -                        return ret;
> +                data->regulator = devm_regulator_get_optional(&pdev->dev, "vtmu");
> +                if (!IS_ERR(data->regulator)) {
> +                        ret = regulator_enable(data->regulator);
> +                        if (ret) {
> +                                dev_err(&pdev->dev, "failed to enable vtmu\n");
> +                                return ret;
> +                        }
> +                } else {
> +                        if (PTR_ERR(data->regulator) == -EPROBE_DEFER)
> +                                return -EPROBE_DEFER;
> +                        dev_info(&pdev->dev, "Regulator node (vtmu) not found\n");
>                  }
> -        } else {
> -                if (PTR_ERR(data->regulator) == -EPROBE_DEFER)
> -                        return -EPROBE_DEFER;
> -                dev_info(&pdev->dev, "Regulator node (vtmu) not found\n");
>          }
>  
>          ret = exynos_map_dt_data(pdev);
>          if (ret)
>                  goto err_sensor;
>  
> -        INIT_WORK(&data->irq_work, exynos_tmu_work);
> +        if (data->soc == SOC_ARCH_ARTPEC8) {
> +                ret = artpec8_register_tzd(pdev);
> +                if (ret)
> +                        return -EINVAL;
> +
> +                INIT_WORK(&data->irq_work, artpec8_tmu_work);
> +        } else {
> +                INIT_WORK(&data->irq_work, exynos_tmu_work);
> +        }

No, this should be parametrized via ops in exynos_tmu_data (one more op
for "work").

>  
>          data->clk = devm_clk_get(&pdev->dev, "tmu_apbif");
>          if (IS_ERR(data->clk)) {
> @@ -1046,18 +1590,21 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>                  goto err_sensor;
>          }
>  
> -        data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
> -        if (IS_ERR(data->clk_sec)) {
> -                if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
> -                        dev_err(&pdev->dev, "Failed to get triminfo clock\n");
> -                        ret = PTR_ERR(data->clk_sec);
> -                        goto err_sensor;
> -                }
> -        } else {
> -                ret = clk_prepare(data->clk_sec);
> -                if (ret) {
> -                        dev_err(&pdev->dev, "Failed to get clock\n");
> -                        goto err_sensor;
> +        if (data->soc != SOC_ARCH_ARTPEC8) {
> +                data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
> +                if (IS_ERR(data->clk_sec)) {
> +                        if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
> +                                dev_err(&pdev->dev,
> +                                        "Failed to get triminfo clock\n");
> +                                ret = PTR_ERR(data->clk_sec);
> +                                goto err_sensor;
> +                        }
> +                } else {
> +                        ret = clk_prepare(data->clk_sec);
> +                        if (ret) {
> +                                dev_err(&pdev->dev, "Failed to get clock\n");
> +                                goto err_sensor;
> +                        }
>                  }
>          }
>  
> @@ -1070,6 +1617,7 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>          switch (data->soc) {
>          case SOC_ARCH_EXYNOS5433:
>          case SOC_ARCH_EXYNOS7:
> +        case SOC_ARCH_ARTPEC8:
>                  data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk");
>                  if (IS_ERR(data->sclk)) {
>                          dev_err(&pdev->dev, "Failed to get sclk\n");
> @@ -1087,24 +1635,26 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>                  break;
>          }
>  
> +        if (data->soc != SOC_ARCH_ARTPEC8) {
>          /*
>           * data->tzd must be registered before calling exynos_tmu_initialize(),
>           * requesting irq and calling exynos_tmu_control().
>           */

Again wrong indentation.


Best regards,
Krzysztof