Re: [PATCH v2 1/8] arm64: dts: mt8183: add thermal zone node

Re: [PATCH v2 1/8] arm64: dts: mt8183: add thermal zone node

[Hsin-Yi Wang]

On Fri, May 10, 2019 at 9:27 PM michael.kao <michael.kao@mediatek.com> wrote:

> +
> +                       tzts1: tzts1 {
> +                               polling-delay-passive = <0>;
> +                               polling-delay = <0>;
> +                               thermal-sensors = <&thermal 1>;
> +                               sustainable-power = <0>;
> +                               trips {};
> +                               cooling-maps {};
> +                       };
> +
Is 0 a valid initial sustainable-power setting? Since we'll still get
warning[1] about this, though it might not be harmful.

If 0 is a valid setting, maybe we should consider showing the warning
of not setting this property in [2]?

[1] https://elixir.bootlin.com/linux/latest/source/drivers/thermal/power_allocator.c#L570
[2] https://elixir.bootlin.com/linux/latest/source/drivers/thermal/of-thermal.c#L1049

Re: [PATCH v2 1/8] arm64: dts: mt8183: add thermal zone node

[Matthias Kaehlcke]

On Thu, May 30, 2019 at 02:27:28PM +0800, Hsin-Yi Wang wrote:
> On Fri, May 10, 2019 at 9:27 PM michael.kao <michael.kao@mediatek.com> wrote:
> 
> > +
> > +                       tzts1: tzts1 {
> > +                               polling-delay-passive = <0>;
> > +                               polling-delay = <0>;
> > +                               thermal-sensors = <&thermal 1>;
> > +                               sustainable-power = <0>;
> > +                               trips {};
> > +                               cooling-maps {};
> > +                       };
> > +
> Is 0 a valid initial sustainable-power setting? Since we'll still get
> warning[1] about this, though it might not be harmful.
> 
> If 0 is a valid setting, maybe we should consider showing the warning
> of not setting this property in [2]?
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/thermal/power_allocator.c#L570
> [2] https://elixir.bootlin.com/linux/latest/source/drivers/thermal/of-thermal.c#L1049

IIUC a value of 0 is pointless, the thermal framework will still use
an estimated value:

https://elixir.bootlin.com/linux/v5.1.5/source/drivers/thermal/power_allocator.c#L203

As commented on v1 (https://patchwork.kernel.org/patch/10926519/#22620905)
the value of the property may depend on the thermal characteristics of
the device, there is not one correct value per SoC/core. If it is
specified at SoC level device makers should be aware that they might
have to override it for 'optimal' behavior on their device.

RE: [PATCH v2 1/8] arm64: dts: mt8183: add thermal zone node

[Michael Kao (高振翔)]

-----Original Message-----
From: Matthias Kaehlcke [mailto:mka@chromium.org] 
Sent: Friday, May 31, 2019 12:08 AM
To: Hsin-Yi Wang <hsinyi@chromium.org>
Cc: Michael Kao (高振翔) <Michael.Kao@mediatek.com>; Zhang Rui <rui.zhang@intel.com>; Eduardo Valentin <edubezval@gmail.com>; Daniel Lezcano <daniel.lezcano@linaro.org>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Matthias Brugger <matthias.bgg@gmail.com>; linux-pm@vger.kernel.org; devicetree@vger.kernel.org; lkml <linux-kernel@vger.kernel.org>; moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v2 1/8] arm64: dts: mt8183: add thermal zone node

On Thu, May 30, 2019 at 02:27:28PM +0800, Hsin-Yi Wang wrote:
> On Fri, May 10, 2019 at 9:27 PM michael.kao <michael.kao@mediatek.com> wrote:
> 
> > +
> > +                       tzts1: tzts1 {
> > +                               polling-delay-passive = <0>;
> > +                               polling-delay = <0>;
> > +                               thermal-sensors = <&thermal 1>;
> > +                               sustainable-power = <0>;
> > +                               trips {};
> > +                               cooling-maps {};
> > +                       };
> > +
> Is 0 a valid initial sustainable-power setting? Since we'll still get 
> warning[1] about this, though it might not be harmful.
> 
> If 0 is a valid setting, maybe we should consider showing the warning 
> of not setting this property in [2]?
> 
> [1] 
> https://elixir.bootlin.com/linux/latest/source/drivers/thermal/power_a
> llocator.c#L570 [2] 
> https://elixir.bootlin.com/linux/latest/source/drivers/thermal/of-ther
> mal.c#L1049

IIUC a value of 0 is pointless, the thermal framework will still use an estimated value:

https://elixir.bootlin.com/linux/v5.1.5/source/drivers/thermal/power_allocator.c#L203

As commented on v1 (https://patchwork.kernel.org/patch/10926519/#22620905)
the value of the property may depend on the thermal characteristics of the device, there is not one correct value per SoC/core. If it is specified at SoC level device makers should be aware that they might have to override it for 'optimal' behavior on their device.

I think there is no need to set sustainable-power for tzts1~6.
They don't bind thermal instance and will not thermal throttle.
So it will be also zero if we don't set sustainable-power and let it use estimate value.
For the purpose of preventing warning, I think I can set sustainable-power of tzts1~6 the same to cpu_thermal.

Re: [PATCH v2 7/8] thermal: mediatek: add another get_temp ops for thermal sensors

[Hsin-Yi Wang]

On Fri, May 10, 2019 at 9:27 PM michael.kao <michael.kao@mediatek.com> wrote:
>
> From: Michael Kao <michael.kao@mediatek.com>
>
> Provide thermal zone to read thermal sensor
> in the SoC. We can read all the thermal sensors
> value in the SoC by the node /sys/class/thermal/
>
> Signed-off-by: Michael Kao <michael.kao@mediatek.com>
> ---
>  drivers/thermal/mtk_thermal.c | 68 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 60 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
> index cb41e46..d5c78b0 100644
> --- a/drivers/thermal/mtk_thermal.c
> +++ b/drivers/thermal/mtk_thermal.c
> @@ -230,6 +230,11 @@ enum {
>
>  struct mtk_thermal;
>
> +struct mtk_thermal_zone {
> +       struct mtk_thermal *mt;
> +       int id;
> +};
> +
>  struct thermal_bank_cfg {
>         unsigned int num_sensors;
>         const int *sensors;
> @@ -612,7 +617,7 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
>                  * not immediately shut down.
>                  */
>                 if (temp > 200000)
> -                       temp = 0;
> +                       temp = -EACCES;
Should we return -EAGAIN here? Otherwise there would be warning:
[    0.344167] thermal thermal_zone5: failed to read out thermal zone (-13)

Since first read is bogus value, I think EAGAIN also makes sense here.
>
>                 if (temp > max)
>                         max = temp;
> @@ -623,7 +628,8 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
>
>  static int mtk_read_temp(void *data, int *temperature)
>  {
> -       struct mtk_thermal *mt = data;
> +       struct mtk_thermal_zone *tz = data;
> +       struct mtk_thermal *mt = tz->mt;
>         int i;
>         int tempmax = INT_MIN;
>
> @@ -636,16 +642,48 @@ static int mtk_read_temp(void *data, int *temperature)
>
>                 mtk_thermal_put_bank(bank);
>         }
> -
>         *temperature = tempmax;
>
>         return 0;
>  }
>
> +static int mtk_read_sensor_temp(void *data, int *temperature)
> +{
> +       struct mtk_thermal_zone *tz = data;
> +       struct mtk_thermal *mt = tz->mt;
> +       const struct mtk_thermal_data *conf = mt->conf;
> +       int id = tz->id - 1;
> +       int temp = INT_MIN;
> +       u32 raw;
> +
> +       if (id < 0)
> +               return  -EACCES;
> +
> +       raw = readl(mt->thermal_base + conf->msr[id]);
> +
> +       temp = raw_to_mcelsius(mt, id, raw);
> +
> +       /*
> +        * The first read of a sensor often contains very high bogus
> +        * temperature value. Filter these out so that the system does
> +        * not immediately shut down.
> +        */
> +
> +       if (temp > 200000)
> +               return  -EACCES;
Same here.
> +
> +       *temperature = temp;
> +       return 0;
> +}
> +
>  static const struct thermal_zone_of_device_ops mtk_thermal_ops = {
>         .get_temp = mtk_read_temp,
>  };
>
> +static const struct thermal_zone_of_device_ops mtk_thermal_sensor_ops = {
> +       .get_temp = mtk_read_sensor_temp,
> +};
> +
>  static void mtk_thermal_init_bank(struct mtk_thermal *mt, int num,
>                                   u32 apmixed_phys_base, u32 auxadc_phys_base,
>                                   int ctrl_id)
> @@ -878,6 +916,7 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>         struct resource *res;
>         u64 auxadc_phys_base, apmixed_phys_base;
>         struct thermal_zone_device *tzdev;
> +       struct mtk_thermal_zone *tz;
>
>         mt = devm_kzalloc(&pdev->dev, sizeof(*mt), GFP_KERNEL);
>         if (!mt)
> @@ -959,11 +998,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, mt);
>
> -       tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, mt,
> -                                                    &mtk_thermal_ops);
> -       if (IS_ERR(tzdev)) {
> -               ret = PTR_ERR(tzdev);
> -               goto err_disable_clk_peri_therm;
> +       for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> +               tz = kmalloc(sizeof(*tz), GFP_KERNEL);
> +               if (!tz)
> +                       return -ENOMEM;
> +
> +               tz->mt = mt;
> +               tz->id = i;
> +
> +               tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, i,
> +                               tz, (i == 0) ?
> +                               &mtk_thermal_ops : &mtk_thermal_sensor_ops);
> +
> +               if (IS_ERR(tzdev)) {
> +                       if (IS_ERR(tzdev) != -EACCES) {
> +                               ret = PTR_ERR(tzdev);
> +                               goto err_disable_clk_peri_therm;
> +                       }
> +               }
>         }
>
>         return 0;
> --
> 2.6.4
>

Re: [PATCH v2 7/8] thermal: mediatek: add another get_temp ops for thermal sensors

[Hsin-Yi Wang]

On Fri, May 10, 2019 at 9:27 PM michael.kao <michael.kao@mediatek.com> wrote:

> -       tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, mt,
> -                                                    &mtk_thermal_ops);
> -       if (IS_ERR(tzdev)) {
> -               ret = PTR_ERR(tzdev);
> -               goto err_disable_clk_peri_therm;
> +       for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> +               tz = kmalloc(sizeof(*tz), GFP_KERNEL);
> +               if (!tz)
> +                       return -ENOMEM;
> +
> +               tz->mt = mt;
> +               tz->id = i;
> +
> +               tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, i,
> +                               tz, (i == 0) ?
> +                               &mtk_thermal_ops : &mtk_thermal_sensor_ops);
> +
> +               if (IS_ERR(tzdev)) {
> +                       if (IS_ERR(tzdev) != -EACCES) {
                                PTR_ERR(tzdev)

> +                               ret = PTR_ERR(tzdev);
> +                               goto err_disable_clk_peri_therm;
> +                       }
> +               }

This for loop adding thermal zone sensors will not work for mt8173. It
assumes that thermal-zones in dts have subnodes (eg. cpu_thermal,
tzts..) amount equal to num_sensors+1. Otherwise tzdev would be
-ENODEV and thermal failed to be probed.
In mt8183 this is fine, since each thermal zone only has one sensor,
but in mt8173, some sensor appears in multiple thermal zones.

In order to let the change also works for 8173, I think if the error
is -ENODEV, and the id is not 0 (0 is cpu_thermal), prompt a warning
instead of failing. Eg.

                if (IS_ERR(tzdev)) {
+                       if (i > 0 && PTR_ERR(tzdev) == -ENODEV) {
+                               dev_warn(&pdev->dev, "can't find
thermal sensor %d\n", i);
+                               continue;
+                       }
                        if (PTR_ERR(tzdev) != -EACCES) {

Re: [PATCH v2 7/8] thermal: mediatek: add another get_temp ops for thermal sensors

[Michael Kao]

On Thu, 2019-11-21 at 15:00 +0800, Hsin-Yi Wang wrote:
> On Fri, May 10, 2019 at 9:27 PM michael.kao <michael.kao@mediatek.com> wrote:
> 
> > -       tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, mt,
> > -                                                    &mtk_thermal_ops);
> > -       if (IS_ERR(tzdev)) {
> > -               ret = PTR_ERR(tzdev);
> > -               goto err_disable_clk_peri_therm;
> > +       for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> > +               tz = kmalloc(sizeof(*tz), GFP_KERNEL);
> > +               if (!tz)
> > +                       return -ENOMEM;
> > +
> > +               tz->mt = mt;
> > +               tz->id = i;
> > +
> > +               tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, i,
> > +                               tz, (i == 0) ?
> > +                               &mtk_thermal_ops : &mtk_thermal_sensor_ops);
> > +
> > +               if (IS_ERR(tzdev)) {
> > +                       if (IS_ERR(tzdev) != -EACCES) {
>                                 PTR_ERR(tzdev)
> 
> > +                               ret = PTR_ERR(tzdev);
> > +                               goto err_disable_clk_peri_therm;
> > +                       }
> > +               }
> 
> This for loop adding thermal zone sensors will not work for mt8173. It
> assumes that thermal-zones in dts have subnodes (eg. cpu_thermal,
> tzts..) amount equal to num_sensors+1. Otherwise tzdev would be
> -ENODEV and thermal failed to be probed.
> In mt8183 this is fine, since each thermal zone only has one sensor,
> but in mt8173, some sensor appears in multiple thermal zones.
> 
> In order to let the change also works for 8173, I think if the error
> is -ENODEV, and the id is not 0 (0 is cpu_thermal), prompt a warning
> instead of failing. Eg.
> 
>                 if (IS_ERR(tzdev)) {
> +                       if (i > 0 && PTR_ERR(tzdev) == -ENODEV) {
> +                               dev_warn(&pdev->dev, "can't find
> thermal sensor %d\n", i);
> +                               continue;
> +                       }
>                         if (PTR_ERR(tzdev) != -EACCES) {

OK, I will update this patch.

Re: [PATCH v2 0/8] Add Mediatek thermal dirver and dtsi

[Daniel Lezcano]

What is the status of this series? Shouldn't we have a V3?

On 10/05/2019 15:26, michael.kao wrote:
> This patchset supports for MT8183 chip to mtk_thermal.c.
> Add thermal zone of all the thermal sensor in SoC for
> another get temperatrue. They don't need to thermal throttle.
> And we bind coolers for thermal zone nodes of cpu_thermal.
> 
> This patch series base on these patches [1][2][3][4].
> 
> [1]support for reading chip ID and efuse (https://patchwork.kernel.org/patch/10902131/)
> [2]arm64: dts: mt8183: Add reset-cells in infracfg (https://patchwork.kernel.org/patch/10908653/)
> [3]clk: reset: Modify reset-controller driver (https://patchwork.kernel.org/patch/10908657/)
> [4]PM / AVS: SVS: Introduce SVS engine (https://patchwork.kernel.org/patch/10923289/)
> 
> Matthias Kaehlcke (2):
> 	arm64: dts: mt8183: Configure CPU cooling
> 	arm64: dts: mt8183: Increase polling frequency for CPU thermal zone
> 
> Michael Kao (6):
> 	arm64: dts: mt8183: add thermal zone node
> 	arm64: dts: mt8183: add/update dynamic power coefficients
> 	arm64: dts: mt8183: Add #cooling-cells to CPU nodes
> 	thermal: mediatek: mt8183: fix bank number settings
> 	thermal: mediatek: add another get_temp ops for thermal sensors
> 	thermal: mediatek: use spinlock to protect PTPCORESEL
> 
> arch/arm64/boot/dts/mediatek/mt8183.dtsi | 158 +++++++++++++++++++++++++++++++
> drivers/thermal/mtk_thermal.c            |  82 +++++++++++++---
> 2 files changed, 226 insertions(+), 14 deletions(-)
> 


-- 
 <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