[PATCH 1/5] thermal: exynos: enable core tmu clk on exynos platform

[PATCH 1/5] thermal: exynos: enable core tmu clk on exynos platform

[Anand Moon]

clk_summary do not show tmu_apbif clk enable, so replace
the clk_prepare with clk_prepare_enables to enable tmu clk.
simplify the enable of tmu_triminfo_apbif clk, also fixed
the order of goto error for failed cases.

CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
before:
cat /sys/kernel/debug/clk/clk_summary | grep tmu
                         tmu_gpu           0            2    66600000          0 0
                         tmu              0            6    66600000          0 0
after:
cat /sys/kernel/debug/clk/clk_summary | grep tmu
                         tmu_gpu       2        2        0    66600000          0 0
                         tmu          6        6        0    66600000          0 0
---
 drivers/thermal/samsung/exynos_tmu.c | 45 ++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index a992e51..0164c9e 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1060,41 +1060,40 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to get clock\n");
 		ret = PTR_ERR(data->clk);
 		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);
+		ret = clk_prepare_enable(data->clk);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to get clock\n");
 			goto err_sensor;
 		}
 	}
 
-	ret = clk_prepare(data->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to get clock\n");
-		goto err_clk_sec;
-	}
-
 	switch (data->soc) {
+	case SOC_ARCH_EXYNOS5420_TRIMINFO:
+		data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
+		if (IS_ERR(data->clk_sec)) {
+			dev_err(&pdev->dev, "Failed to get triminfo clock\n");
+			ret = PTR_ERR(data->clk_sec);
+			goto err_clk;
+		} else {
+			ret = clk_prepare_enable(data->clk_sec);
+			if (ret) {
+				dev_err(&pdev->dev, "Failed to get clock\n");
+				goto err_clk;
+			}
+		}
+		break;
 	case SOC_ARCH_EXYNOS5433:
 	case SOC_ARCH_EXYNOS7:
 		data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk");
 		if (IS_ERR(data->sclk)) {
 			dev_err(&pdev->dev, "Failed to get sclk\n");
-			goto err_clk;
+			goto err_clk_sec;
 		} else {
 			ret = clk_prepare_enable(data->sclk);
 			if (ret) {
 				dev_err(&pdev->dev, "Failed to enable sclk\n");
-				goto err_clk;
+				goto err_clk_sec;
 			}
 		}
 		break;
@@ -1134,11 +1133,11 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 	thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
 err_sclk:
 	clk_disable_unprepare(data->sclk);
-err_clk:
-	clk_unprepare(data->clk);
 err_clk_sec:
 	if (!IS_ERR(data->clk_sec))
-		clk_unprepare(data->clk_sec);
+		clk_disable_unprepare(data->clk_sec);
+err_clk:
+	clk_disable_unprepare(data->clk);
 err_sensor:
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
@@ -1155,9 +1154,9 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 	exynos_tmu_control(pdev, false);
 
 	clk_disable_unprepare(data->sclk);
-	clk_unprepare(data->clk);
+	clk_disable_unprepare(data->clk);
 	if (!IS_ERR(data->clk_sec))
-		clk_unprepare(data->clk_sec);
+		clk_disable_unprepare(data->clk_sec);
 
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
-- 
2.7.4

[PATCH 2/5] thermal: exynos: cleanup of clk err check for exynos_tmu_work

[Anand Moon]

cleanup err check in exynos_tmu_work as clk internal
framework will perform if clk is enable/disable
so drop the double check of IS_ERR and other such references.

CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 0164c9e..2dbde97 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -300,8 +300,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 
 	mutex_lock(&data->lock);
 	clk_enable(data->clk);
-	if (!IS_ERR(data->clk_sec))
-		clk_enable(data->clk_sec);
+	clk_enable(data->clk_sec);
 
 	status = readb(data->base + EXYNOS_TMU_REG_STATUS);
 	if (!status) {
@@ -334,8 +333,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 err:
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
-	if (!IS_ERR(data->clk_sec))
-		clk_disable(data->clk_sec);
+	clk_disable(data->clk_sec);
 out:
 	return ret;
 }
@@ -789,19 +787,16 @@ static void exynos_tmu_work(struct work_struct *work)
 	struct exynos_tmu_data *data = container_of(work,
 			struct exynos_tmu_data, irq_work);
 
-	if (!IS_ERR(data->clk_sec))
-		clk_enable(data->clk_sec);
-	if (!IS_ERR(data->clk_sec))
-		clk_disable(data->clk_sec);
-
 	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
 
 	mutex_lock(&data->lock);
 	clk_enable(data->clk);
+	clk_enable(data->clk_sec);
 
 	/* TODO: take action based on particular interrupt */
 	data->tmu_clear_irqs(data);
 
+	clk_disable(data->clk_sec);
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 	enable_irq(data->irq);
@@ -1134,8 +1129,7 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 err_sclk:
 	clk_disable_unprepare(data->sclk);
 err_clk_sec:
-	if (!IS_ERR(data->clk_sec))
-		clk_disable_unprepare(data->clk_sec);
+	clk_disable_unprepare(data->clk_sec);
 err_clk:
 	clk_disable_unprepare(data->clk);
 err_sensor:
@@ -1155,8 +1149,7 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(data->sclk);
 	clk_disable_unprepare(data->clk);
-	if (!IS_ERR(data->clk_sec))
-		clk_disable_unprepare(data->clk_sec);
+	clk_disable_unprepare(data->clk_sec);
 
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
-- 
2.7.4

Re: [PATCH 2/5] thermal: exynos: cleanup of clk err check for exynos_tmu_work

[Krzysztof Kozlowski]

On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
> cleanup err check in exynos_tmu_work as clk internal
> framework will perform if clk is enable/disable
> so drop the double check of IS_ERR and other such references.

I do not understand the statement. Clock framework will perform if clk
is enable/disable? How clock can be "enable" or "disable"? You mean
gate clock? you mean clock pointer is an ERR pointer?

> CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 0164c9e..2dbde97 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -300,8 +300,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>
>         mutex_lock(&data->lock);
>         clk_enable(data->clk);
> -       if (!IS_ERR(data->clk_sec))
> -               clk_enable(data->clk_sec);
> +       clk_enable(data->clk_sec);
>
>         status = readb(data->base + EXYNOS_TMU_REG_STATUS);
>         if (!status) {
> @@ -334,8 +333,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>  err:
>         clk_disable(data->clk);
>         mutex_unlock(&data->lock);
> -       if (!IS_ERR(data->clk_sec))
> -               clk_disable(data->clk_sec);
> +       clk_disable(data->clk_sec);
>  out:
>         return ret;
>  }
> @@ -789,19 +787,16 @@ static void exynos_tmu_work(struct work_struct *work)
>         struct exynos_tmu_data *data = container_of(work,
>                         struct exynos_tmu_data, irq_work);
>
> -       if (!IS_ERR(data->clk_sec))
> -               clk_enable(data->clk_sec);
> -       if (!IS_ERR(data->clk_sec))
> -               clk_disable(data->clk_sec);
> -
>         thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
>
>         mutex_lock(&data->lock);
>         clk_enable(data->clk);
> +       clk_enable(data->clk_sec);

You are changing here the logic completely. Before the "enable" was
followed immediately by "disable". Now you are moving disable
somewhere else... All this looks suspicious...

Best regards,
Krzysztof

>
>         /* TODO: take action based on particular interrupt */
>         data->tmu_clear_irqs(data);
>
> +       clk_disable(data->clk_sec);
>         clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>         enable_irq(data->irq);
> @@ -1134,8 +1129,7 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>  err_sclk:
>         clk_disable_unprepare(data->sclk);
>  err_clk_sec:
> -       if (!IS_ERR(data->clk_sec))
> -               clk_disable_unprepare(data->clk_sec);
> +       clk_disable_unprepare(data->clk_sec);
>  err_clk:
>         clk_disable_unprepare(data->clk);
>  err_sensor:
> @@ -1155,8 +1149,7 @@ static int exynos_tmu_remove(struct platform_device *pdev)
>
>         clk_disable_unprepare(data->sclk);
>         clk_disable_unprepare(data->clk);
> -       if (!IS_ERR(data->clk_sec))
> -               clk_disable_unprepare(data->clk_sec);
> +       clk_disable_unprepare(data->clk_sec);
>
>         if (!IS_ERR(data->regulator))
>                 regulator_disable(data->regulator);
> --
> 2.7.4
>

Re: [PATCH 2/5] thermal: exynos: cleanup of clk err check for exynos_tmu_work

[Anand Moon]

Hi Krzysztof,

On 17 July 2018 at 17:54, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>> cleanup err check in exynos_tmu_work as clk internal
>> framework will perform if clk is enable/disable
>> so drop the double check of IS_ERR and other such references.
>
> I do not understand the statement. Clock framework will perform if clk
> is enable/disable? How clock can be "enable" or "disable"? You mean
> gate clock? you mean clock pointer is an ERR pointer?
>

if (!IS_ERR(data->clk_sec))
check if the pointer is valid or not
this check is again performed in
clk_enable.

>> CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> ---
>>  drivers/thermal/samsung/exynos_tmu.c | 19 ++++++-------------
>>  1 file changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>> index 0164c9e..2dbde97 100644
>> --- a/drivers/thermal/samsung/exynos_tmu.c
>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>> @@ -300,8 +300,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>
>>         mutex_lock(&data->lock);
>>         clk_enable(data->clk);
>> -       if (!IS_ERR(data->clk_sec))
>> -               clk_enable(data->clk_sec);
>> +       clk_enable(data->clk_sec);
>>
>>         status = readb(data->base + EXYNOS_TMU_REG_STATUS);
>>         if (!status) {
>> @@ -334,8 +333,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>  err:
>>         clk_disable(data->clk);
>>         mutex_unlock(&data->lock);
>> -       if (!IS_ERR(data->clk_sec))
>> -               clk_disable(data->clk_sec);
>> +       clk_disable(data->clk_sec);
>>  out:
>>         return ret;
>>  }
>> @@ -789,19 +787,16 @@ static void exynos_tmu_work(struct work_struct *work)
>>         struct exynos_tmu_data *data = container_of(work,
>>                         struct exynos_tmu_data, irq_work);
>>
>> -       if (!IS_ERR(data->clk_sec))
>> -               clk_enable(data->clk_sec);
>> -       if (!IS_ERR(data->clk_sec))
>> -               clk_disable(data->clk_sec);
>> -
>>         thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
>>
>>         mutex_lock(&data->lock);
>>         clk_enable(data->clk);
>> +       clk_enable(data->clk_sec);
>
> You are changing here the logic completely. Before the "enable" was
> followed immediately by "disable". Now you are moving disable
> somewhere else... All this looks suspicious...

I chose to move enable/disable of clk_sec this under the mutex lock for safe
which dose the same sequence with different order.

Second approach:
We should get rid of clk_enable/disable in exynos_tmu_work
as this looks unnecessary for toggle clk's on every update.

Best Regards
-Anand

Re: [PATCH 2/5] thermal: exynos: cleanup of clk err check for exynos_tmu_work

[Krzysztof Kozlowski]

On 17 July 2018 at 22:08, Anand Moon <linux.amoon@gmail.com> wrote:
> Hi Krzysztof,
>
> On 17 July 2018 at 17:54, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>>> cleanup err check in exynos_tmu_work as clk internal
>>> framework will perform if clk is enable/disable
>>> so drop the double check of IS_ERR and other such references.
>>
>> I do not understand the statement. Clock framework will perform if clk
>> is enable/disable? How clock can be "enable" or "disable"? You mean
>> gate clock? you mean clock pointer is an ERR pointer?
>>
>
> if (!IS_ERR(data->clk_sec))
> check if the pointer is valid or not
> this check is again performed in
> clk_enable.

This should be then written in commit msg.

>>> CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>>  drivers/thermal/samsung/exynos_tmu.c | 19 ++++++-------------
>>>  1 file changed, 6 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>> index 0164c9e..2dbde97 100644
>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>> @@ -300,8 +300,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>
>>>         mutex_lock(&data->lock);
>>>         clk_enable(data->clk);
>>> -       if (!IS_ERR(data->clk_sec))
>>> -               clk_enable(data->clk_sec);
>>> +       clk_enable(data->clk_sec);
>>>
>>>         status = readb(data->base + EXYNOS_TMU_REG_STATUS);
>>>         if (!status) {
>>> @@ -334,8 +333,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>  err:
>>>         clk_disable(data->clk);
>>>         mutex_unlock(&data->lock);
>>> -       if (!IS_ERR(data->clk_sec))
>>> -               clk_disable(data->clk_sec);
>>> +       clk_disable(data->clk_sec);
>>>  out:
>>>         return ret;
>>>  }
>>> @@ -789,19 +787,16 @@ static void exynos_tmu_work(struct work_struct *work)
>>>         struct exynos_tmu_data *data = container_of(work,
>>>                         struct exynos_tmu_data, irq_work);
>>>
>>> -       if (!IS_ERR(data->clk_sec))
>>> -               clk_enable(data->clk_sec);
>>> -       if (!IS_ERR(data->clk_sec))
>>> -               clk_disable(data->clk_sec);
>>> -
>>>         thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
>>>
>>>         mutex_lock(&data->lock);
>>>         clk_enable(data->clk);
>>> +       clk_enable(data->clk_sec);
>>
>> You are changing here the logic completely. Before the "enable" was
>> followed immediately by "disable". Now you are moving disable
>> somewhere else... All this looks suspicious...
>
> I chose to move enable/disable of clk_sec this under the mutex lock for safe
> which dose the same sequence with different order.
>
> Second approach:
> We should get rid of clk_enable/disable in exynos_tmu_work
> as this looks unnecessary for toggle clk's on every update.

I already sent a cleanup for this:
https://patchwork.kernel.org/patch/10529971/

Best regards,
Krzysztof

[PATCH 3/5] thermal: exynos: increase the number of trips for exynos542x

[Anand Moon]

increase the number of trips to support more polling modes,
changes help supress following warning.

[    4.231731] thermal thermal_zone0: failed to read out thermal zone (-22)
[    4.237190] exynos-tmu 10060000.tmu: More trip points than supported by this TMU.
[    4.244661] exynos-tmu 10060000.tmu: 2 trip points should be configured in polling mode.
[    4.254821] thermal thermal_zone1: failed to read out thermal zone (-22)
[    4.260370] exynos-tmu 10064000.tmu: More trip points than supported by this TMU.
[    4.267631] exynos-tmu 10064000.tmu: 2 trip points should be configured in polling mode.
[    4.277551] thermal thermal_zone2: failed to read out thermal zone (-22)
[    4.283090] exynos-tmu 10068000.tmu: More trip points than supported by this TMU.
[    4.290438] exynos-tmu 10068000.tmu: 2 trip points should be configured in polling mode.
[    4.300280] thermal thermal_zone3: failed to read out thermal zone (-22)
[    4.305711] exynos-tmu 1006c000.tmu: More trip points than supported by this TMU.
[    4.313171] exynos-tmu 1006c000.tmu: 2 trip points should be configured in polling mode.
[    4.323002] thermal thermal_zone4: failed to read out thermal zone (-22)

CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 2dbde97..4a2733c 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -935,7 +935,7 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 		data->tmu_read = exynos4412_tmu_read;
 		data->tmu_set_emulation = exynos4412_tmu_set_emulation;
 		data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
-		data->ntrip = 4;
+		data->ntrip = 8;
 		data->gain = 8;
 		data->reference_voltage = 16;
 		data->efuse_value = 55;
-- 
2.7.4

Re: [PATCH 3/5] thermal: exynos: increase the number of trips for exynos542x

[Krzysztof Kozlowski]

On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
> increase the number of trips to support more polling modes,
> changes help supress following warning.
>
> [    4.231731] thermal thermal_zone0: failed to read out thermal zone (-22)
> [    4.237190] exynos-tmu 10060000.tmu: More trip points than supported by this TMU.
> [    4.244661] exynos-tmu 10060000.tmu: 2 trip points should be configured in polling mode.
> [    4.254821] thermal thermal_zone1: failed to read out thermal zone (-22)
> [    4.260370] exynos-tmu 10064000.tmu: More trip points than supported by this TMU.
> [    4.267631] exynos-tmu 10064000.tmu: 2 trip points should be configured in polling mode.
> [    4.277551] thermal thermal_zone2: failed to read out thermal zone (-22)
> [    4.283090] exynos-tmu 10068000.tmu: More trip points than supported by this TMU.
> [    4.290438] exynos-tmu 10068000.tmu: 2 trip points should be configured in polling mode.
> [    4.300280] thermal thermal_zone3: failed to read out thermal zone (-22)
> [    4.305711] exynos-tmu 1006c000.tmu: More trip points than supported by this TMU.
> [    4.313171] exynos-tmu 1006c000.tmu: 2 trip points should be configured in polling mode.
> [    4.323002] thermal thermal_zone4: failed to read out thermal zone (-22)
>
> CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>

NAK

Exynos5420/5422/5800 has 4 trip points (understood as in current driver design).

Best regards,
Krzysztof

> ---
>  drivers/thermal/samsung/exynos_tmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 2dbde97..4a2733c 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -935,7 +935,7 @@ static int exynos_map_dt_data(struct platform_device *pdev)
>                 data->tmu_read = exynos4412_tmu_read;
>                 data->tmu_set_emulation = exynos4412_tmu_set_emulation;
>                 data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
> -               data->ntrip = 4;
> +               data->ntrip = 8;
>                 data->gain = 8;
>                 data->reference_voltage = 16;
>                 data->efuse_value = 55;
> --
> 2.7.4
>

Re: [PATCH 3/5] thermal: exynos: increase the number of trips for exynos542x

[Anand Moon]

Hi Krzysztof,

On 17 July 2018 at 17:55, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>> increase the number of trips to support more polling modes,
>> changes help supress following warning.
>>
>> [    4.231731] thermal thermal_zone0: failed to read out thermal zone (-22)
>> [    4.237190] exynos-tmu 10060000.tmu: More trip points than supported by this TMU.
>> [    4.244661] exynos-tmu 10060000.tmu: 2 trip points should be configured in polling mode.
>> [    4.254821] thermal thermal_zone1: failed to read out thermal zone (-22)
>> [    4.260370] exynos-tmu 10064000.tmu: More trip points than supported by this TMU.
>> [    4.267631] exynos-tmu 10064000.tmu: 2 trip points should be configured in polling mode.
>> [    4.277551] thermal thermal_zone2: failed to read out thermal zone (-22)
>> [    4.283090] exynos-tmu 10068000.tmu: More trip points than supported by this TMU.
>> [    4.290438] exynos-tmu 10068000.tmu: 2 trip points should be configured in polling mode.
>> [    4.300280] thermal thermal_zone3: failed to read out thermal zone (-22)
>> [    4.305711] exynos-tmu 1006c000.tmu: More trip points than supported by this TMU.
>> [    4.313171] exynos-tmu 1006c000.tmu: 2 trip points should be configured in polling mode.
>> [    4.323002] thermal thermal_zone4: failed to read out thermal zone (-22)
>>
>> CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>
> NAK
>
> Exynos5420/5422/5800 has 4 trip points (understood as in current driver design).
>

Ok no issue.

Best Regards
-Anand

[PATCH 4/5] thermal: exynos: fixed the efuse min/max value for exynos5422

[Anand Moon]

e-fuse range min~max range is 16~76. if e-fuse value is out of
this range, then thermal sensor may not sense thermal data properly.

CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 4a2733c..6481d91 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -940,11 +940,13 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 		data->reference_voltage = 16;
 		data->efuse_value = 55;
 		if (data->soc != SOC_ARCH_EXYNOS5420 &&
-		    data->soc != SOC_ARCH_EXYNOS5420_TRIMINFO)
+		    data->soc != SOC_ARCH_EXYNOS5420_TRIMINFO) {
+			data->min_efuse_value = 16;
+			data->max_efuse_value = 76;
+		} else {
 			data->min_efuse_value = 40;
-		else
-			data->min_efuse_value = 0;
-		data->max_efuse_value = 100;
+			data->max_efuse_value = 100;
+		}
 		break;
 	case SOC_ARCH_EXYNOS5433:
 		data->tmu_set_trip_temp = exynos5433_tmu_set_trip_temp;
-- 
2.7.4

Re: [PATCH 4/5] thermal: exynos: fixed the efuse min/max value for exynos5422

[Krzysztof Kozlowski]

On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
> e-fuse range min~max range is 16~76. if e-fuse value is out of

s/e-fuse/Fuse/
s/if/If/

Can you share the sources confirming that fuse values are between 16 and 76?

Best regards,
Krzysztof

> this range, then thermal sensor may not sense thermal data properly.
>
> CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 4a2733c..6481d91 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -940,11 +940,13 @@ static int exynos_map_dt_data(struct platform_device *pdev)
>                 data->reference_voltage = 16;
>                 data->efuse_value = 55;
>                 if (data->soc != SOC_ARCH_EXYNOS5420 &&
> -                   data->soc != SOC_ARCH_EXYNOS5420_TRIMINFO)
> +                   data->soc != SOC_ARCH_EXYNOS5420_TRIMINFO) {
> +                       data->min_efuse_value = 16;
> +                       data->max_efuse_value = 76;
> +               } else {
>                         data->min_efuse_value = 40;
> -               else
> -                       data->min_efuse_value = 0;
> -               data->max_efuse_value = 100;
> +                       data->max_efuse_value = 100;
> +               }
>                 break;
>         case SOC_ARCH_EXYNOS5433:
>                 data->tmu_set_trip_temp = exynos5433_tmu_set_trip_temp;
> --
> 2.7.4
>

Re: [PATCH 4/5] thermal: exynos: fixed the efuse min/max value for exynos5422

[Anand Moon]

Hi Krzysztof,

On 17 July 2018 at 17:58, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>> e-fuse range min~max range is 16~76. if e-fuse value is out of
>
> s/e-fuse/Fuse/
> s/if/If/
>
> Can you share the sources confirming that fuse values are between 16 and 76?
>
Below is my patch :
https://github.com/hardkernel/linux/commit/52c4c9b295930bc53b215d38e274aeb0780917f1

e-Fuse range min/max values are from exynos5422 user manual.

Best Regards
-Anand

Re: [PATCH 4/5] thermal: exynos: fixed the efuse min/max value for exynos5422

[Krzysztof Kozlowski]

On 17 July 2018 at 21:58, Anand Moon <linux.amoon@gmail.com> wrote:
> Hi Krzysztof,
>
> On 17 July 2018 at 17:58, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>>> e-fuse range min~max range is 16~76. if e-fuse value is out of
>>
>> s/e-fuse/Fuse/
>> s/if/If/
>>
>> Can you share the sources confirming that fuse values are between 16 and 76?
>>
> Below is my patch :
> https://github.com/hardkernel/linux/commit/52c4c9b295930bc53b215d38e274aeb0780917f1
>
> e-Fuse range min/max values are from exynos5422 user manual.

Indeed. Can you mention in commit msg, User Manual as the reference of this?

Best regards,
Krzysztof

[PATCH 5/5] ARM: dts: exynos: add tmu aliases nodes

[Anand Moon]

add tmuctrl aliases node for exynos542x

CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 arch/arm/boot/dts/exynos5420-peach-pit.dts         |  8 ++++----
 arch/arm/boot/dts/exynos5420.dtsi                  | 20 ++++++++++++--------
 arch/arm/boot/dts/exynos5422-odroid-core.dtsi      |  8 ++++----
 arch/arm/boot/dts/exynos5422-odroidhc1.dts         |  8 ++++----
 arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi |  8 ++++----
 arch/arm/boot/dts/exynos5800-peach-pi.dts          |  8 ++++----
 6 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
index 57c2332..ff79f0b 100644
--- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
@@ -1061,19 +1061,19 @@
 	status = "okay";
 };
 
-&tmu_cpu0 {
+&tmu_cpu_0 {
 	vtmu-supply = <&ldo10_reg>;
 };
 
-&tmu_cpu1 {
+&tmu_cpu_1 {
 	vtmu-supply = <&ldo10_reg>;
 };
 
-&tmu_cpu2 {
+&tmu_cpu_2 {
 	vtmu-supply = <&ldo10_reg>;
 };
 
-&tmu_cpu3 {
+&tmu_cpu_3 {
 	vtmu-supply = <&ldo10_reg>;
 };
 
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index f4e8c58..c0441ca 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -35,6 +35,10 @@
 		spi0 = &spi_0;
 		spi1 = &spi_1;
 		spi2 = &spi_2;
+		tmuctrl0 = &tmu_cpu_0;
+		tmuctrl1 = &tmu_cpu_1;
+		tmuctrl2 = &tmu_cpu_2;
+		tmuctrl3 = &tmu_cpu_3;
 	};
 
 	/*
@@ -732,7 +736,7 @@
 			interrupt-parent = <&gic>;
 		};
 
-		tmu_cpu0: tmu@10060000 {
+		tmu_cpu_0: tmu@10060000 {
 			compatible = "samsung,exynos5420-tmu";
 			reg = <0x10060000 0x100>;
 			interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
@@ -741,7 +745,7 @@
 			#include "exynos5420-tmu-sensor-conf.dtsi"
 		};
 
-		tmu_cpu1: tmu@10064000 {
+		tmu_cpu_1: tmu@10064000 {
 			compatible = "samsung,exynos5420-tmu";
 			reg = <0x10064000 0x100>;
 			interrupts = <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>;
@@ -750,7 +754,7 @@
 			#include "exynos5420-tmu-sensor-conf.dtsi"
 		};
 
-		tmu_cpu2: tmu@10068000 {
+		tmu_cpu_2: tmu@10068000 {
 			compatible = "samsung,exynos5420-tmu-ext-triminfo";
 			reg = <0x10068000 0x100>, <0x1006c000 0x4>;
 			interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>;
@@ -759,7 +763,7 @@
 			#include "exynos5420-tmu-sensor-conf.dtsi"
 		};
 
-		tmu_cpu3: tmu@1006c000 {
+		tmu_cpu_3: tmu@1006c000 {
 			compatible = "samsung,exynos5420-tmu-ext-triminfo";
 			reg = <0x1006c000 0x100>, <0x100a0000 0x4>;
 			interrupts = <GIC_SPI 185 IRQ_TYPE_LEVEL_HIGH>;
@@ -1341,19 +1345,19 @@
 
 	thermal-zones {
 		cpu0_thermal: cpu0-thermal {
-			thermal-sensors = <&tmu_cpu0>;
+			thermal-sensors = <&tmu_cpu_0>;
 			#include "exynos5420-trip-points.dtsi"
 		};
 		cpu1_thermal: cpu1-thermal {
-		       thermal-sensors = <&tmu_cpu1>;
+		       thermal-sensors = <&tmu_cpu_1>;
 		       #include "exynos5420-trip-points.dtsi"
 		};
 		cpu2_thermal: cpu2-thermal {
-		       thermal-sensors = <&tmu_cpu2>;
+		       thermal-sensors = <&tmu_cpu_2>;
 		       #include "exynos5420-trip-points.dtsi"
 		};
 		cpu3_thermal: cpu3-thermal {
-		       thermal-sensors = <&tmu_cpu3>;
+		       thermal-sensors = <&tmu_cpu_3>;
 		       #include "exynos5420-trip-points.dtsi"
 		};
 		gpu_thermal: gpu-thermal {
diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
index 2f4f408..e28091f 100644
--- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
+++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
@@ -397,19 +397,19 @@
 	};
 };
 
-&tmu_cpu0 {
+&tmu_cpu_0 {
 	vtmu-supply = <&ldo7_reg>;
 };
 
-&tmu_cpu1 {
+&tmu_cpu_1 {
 	vtmu-supply = <&ldo7_reg>;
 };
 
-&tmu_cpu2 {
+&tmu_cpu_2 {
 	vtmu-supply = <&ldo7_reg>;
 };
 
-&tmu_cpu3 {
+&tmu_cpu_3 {
 	vtmu-supply = <&ldo7_reg>;
 };
 
diff --git a/arch/arm/boot/dts/exynos5422-odroidhc1.dts b/arch/arm/boot/dts/exynos5422-odroidhc1.dts
index 8f332be..310222f 100644
--- a/arch/arm/boot/dts/exynos5422-odroidhc1.dts
+++ b/arch/arm/boot/dts/exynos5422-odroidhc1.dts
@@ -29,7 +29,7 @@
 
 	thermal-zones {
 		cpu0_thermal: cpu0-thermal {
-			thermal-sensors = <&tmu_cpu0 0>;
+			thermal-sensors = <&tmu_cpu_0 0>;
 			trips {
 				cpu0_alert0: cpu-alert-0 {
 					temperature = <70000>; /* millicelsius */
@@ -78,7 +78,7 @@
 			};
 		};
 		cpu1_thermal: cpu1-thermal {
-			thermal-sensors = <&tmu_cpu1 0>;
+			thermal-sensors = <&tmu_cpu_1 0>;
 			trips {
 				cpu1_alert0: cpu-alert-0 {
 					temperature = <70000>;
@@ -116,7 +116,7 @@
 			};
 		};
 		cpu2_thermal: cpu2-thermal {
-			thermal-sensors = <&tmu_cpu2 0>;
+			thermal-sensors = <&tmu_cpu_2 0>;
 			trips {
 				cpu2_alert0: cpu-alert-0 {
 					temperature = <70000>;
@@ -154,7 +154,7 @@
 			};
 		};
 		cpu3_thermal: cpu3-thermal {
-			thermal-sensors = <&tmu_cpu3 0>;
+			thermal-sensors = <&tmu_cpu_3 0>;
 			trips {
 				cpu3_alert0: cpu-alert-0 {
 					temperature = <70000>;
diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi b/arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi
index 96e281c..36af2ea 100644
--- a/arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi
+++ b/arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi
@@ -52,7 +52,7 @@
 
 	thermal-zones {
 		cpu0_thermal: cpu0-thermal {
-			thermal-sensors = <&tmu_cpu0 0>;
+			thermal-sensors = <&tmu_cpu_0 0>;
 			polling-delay-passive = <250>;
 			polling-delay = <0>;
 			trips {
@@ -135,7 +135,7 @@
 			};
 		};
 		cpu1_thermal: cpu1-thermal {
-			thermal-sensors = <&tmu_cpu1 0>;
+			thermal-sensors = <&tmu_cpu_1 0>;
 			polling-delay-passive = <250>;
 			polling-delay = <0>;
 			trips {
@@ -202,7 +202,7 @@
 			};
 		};
 		cpu2_thermal: cpu2-thermal {
-			thermal-sensors = <&tmu_cpu2 0>;
+			thermal-sensors = <&tmu_cpu_2 0>;
 			polling-delay-passive = <250>;
 			polling-delay = <0>;
 			trips {
@@ -269,7 +269,7 @@
 			};
 		};
 		cpu3_thermal: cpu3-thermal {
-			thermal-sensors = <&tmu_cpu3 0>;
+			thermal-sensors = <&tmu_cpu_3 0>;
 			polling-delay-passive = <250>;
 			polling-delay = <0>;
 			trips {
diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index d80ab90..c976d02 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -1030,19 +1030,19 @@
 	status = "okay";
 };
 
-&tmu_cpu0 {
+&tmu_cpu_0 {
 	vtmu-supply = <&ldo10_reg>;
 };
 
-&tmu_cpu1 {
+&tmu_cpu_1 {
 	vtmu-supply = <&ldo10_reg>;
 };
 
-&tmu_cpu2 {
+&tmu_cpu_2 {
 	vtmu-supply = <&ldo10_reg>;
 };
 
-&tmu_cpu3 {
+&tmu_cpu_3 {
 	vtmu-supply = <&ldo10_reg>;
 };
 
-- 
2.7.4

Re: [PATCH 5/5] ARM: dts: exynos: add tmu aliases nodes

[Krzysztof Kozlowski]

On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
> add tmuctrl aliases node for exynos542x

Please use full sentence, starting with capital letter and ending with
a dot. Also please explain why you need tmuctrl aliases.

>
> CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  arch/arm/boot/dts/exynos5420-peach-pit.dts         |  8 ++++----
>  arch/arm/boot/dts/exynos5420.dtsi                  | 20 ++++++++++++--------
>  arch/arm/boot/dts/exynos5422-odroid-core.dtsi      |  8 ++++----
>  arch/arm/boot/dts/exynos5422-odroidhc1.dts         |  8 ++++----
>  arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi |  8 ++++----
>  arch/arm/boot/dts/exynos5800-peach-pi.dts          |  8 ++++----
>  6 files changed, 32 insertions(+), 28 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
> index 57c2332..ff79f0b 100644
> --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
> +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
> @@ -1061,19 +1061,19 @@
>         status = "okay";
>  };
>
> -&tmu_cpu0 {
> +&tmu_cpu_0 {

This looks irrelevant to the topic of adding aliases.

Best regards,
Krzysztof

Re: [PATCH 5/5] ARM: dts: exynos: add tmu aliases nodes

[Anand Moon]

Hi

On 17 July 2018 at 17:59, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>> add tmuctrl aliases node for exynos542x
>
> Please use full sentence, starting with capital letter and ending with
> a dot. Also please explain why you need tmuctrl aliases.
>

As per the [0] Documentation/devicetree/bindings/thermal/exynos-thermal.txt

For multi-instance tmu each instance should have an alias correctly
numbered in "aliases" node.

>>
>> CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> ---
>>  arch/arm/boot/dts/exynos5420-peach-pit.dts         |  8 ++++----
>>  arch/arm/boot/dts/exynos5420.dtsi                  | 20 ++++++++++++--------
>>  arch/arm/boot/dts/exynos5422-odroid-core.dtsi      |  8 ++++----
>>  arch/arm/boot/dts/exynos5422-odroidhc1.dts         |  8 ++++----
>>  arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi |  8 ++++----
>>  arch/arm/boot/dts/exynos5800-peach-pi.dts          |  8 ++++----
>>  6 files changed, 32 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> index 57c2332..ff79f0b 100644
>> --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> @@ -1061,19 +1061,19 @@
>>         status = "okay";
>>  };
>>
>> -&tmu_cpu0 {
>> +&tmu_cpu_0 {
>
> This looks irrelevant to the topic of adding aliases.
>

correct, I gone through the binding documentation and hence update this patch.

Best Regards
-Anand

Re: [PATCH 5/5] ARM: dts: exynos: add tmu aliases nodes

[Krzysztof Kozlowski]

On 17 July 2018 at 21:56, Anand Moon <linux.amoon@gmail.com> wrote:
> Hi
>
> On 17 July 2018 at 17:59, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>>> add tmuctrl aliases node for exynos542x
>>
>> Please use full sentence, starting with capital letter and ending with
>> a dot. Also please explain why you need tmuctrl aliases.
>>
>
> As per the [0] Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>
> For multi-instance tmu each instance should have an alias correctly
> numbered in "aliases" node.

Ah, right. However except reading value of alias and storing it, the
driver does not use it later. Maybe this should be just removed.

Best regards,
Krzysztof

Re: [PATCH 1/5] thermal: exynos: enable core tmu clk on exynos platform

[Krzysztof Kozlowski]

Hi Anand,

Thanks for patch.

On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
> clk_summary do not show tmu_apbif clk enable, so replace
> the clk_prepare with clk_prepare_enables to enable tmu clk.

This is not valid reason to do a change. What is clk_summary does not
really matter. Your change has negative impact on power consumption as
the clock stays enabled all the time. This is not what we want... so
please explain it more - why you need the clock to be enabled all the
time? What is broken (clk_summary is not broken in this case)?

> simplify the enable of tmu_triminfo_apbif clk, also fixed
> the order of goto error for failed cases.

This has to be split into separate change.

Best regards,
Krzysztof

Re: [PATCH 1/5] thermal: exynos: enable core tmu clk on exynos platform

[Anand Moon]

Hi Krzysztof

On 17 July 2018 at 17:50, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> Hi Anand,
>
> Thanks for patch.
>
> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>> clk_summary do not show tmu_apbif clk enable, so replace
>> the clk_prepare with clk_prepare_enables to enable tmu clk.
>
> This is not valid reason to do a change. What is clk_summary does not
> really matter. Your change has negative impact on power consumption as
> the clock stays enabled all the time. This is not what we want... so
> please explain it more - why you need the clock to be enabled all the
> time? What is broken (clk_summary is not broken in this case)?
>

Opps I could not explain some more in my commit message.

Actually TMU sensor for Exynos process are controlled by so external clk

Exynos4412 have VDD18_TS sensor which controls the CLK_SENSE tmu.
Exynos5422 have VDD18_TS01 / VDD18_TS23 / VDD18_TS4 sensor which
control the CLK_SENSE tmu.

So as per my understanding tmu is clk driver which control the flow  PMIC.

clk_prepare_enable combine clk_prepare and clk_enable
and clk_disable_unprepare combine clk_disable and clk_unprepare.

most of the driver prefer clk_prepare_enable and clk_disable_unprepare.

clk_summary is just a reference looking point where we could check the
clk is enable/disable.

what is broken ?
I still few more parameter need to tuned to configure the tmu driver.

>> simplify the enable of tmu_triminfo_apbif clk, also fixed
>> the order of goto error for failed cases.
>
> This has to be split into separate change.
>
> Best regards,
> Krzysztof

Best Regards
-Anand

Re: [PATCH 1/5] thermal: exynos: enable core tmu clk on exynos platform

[Krzysztof Kozlowski]

On 17 July 2018 at 22:23, Anand Moon <linux.amoon@gmail.com> wrote:
> Hi Krzysztof
>
> On 17 July 2018 at 17:50, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> Hi Anand,
>>
>> Thanks for patch.
>>
>> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>>> clk_summary do not show tmu_apbif clk enable, so replace
>>> the clk_prepare with clk_prepare_enables to enable tmu clk.
>>
>> This is not valid reason to do a change. What is clk_summary does not
>> really matter. Your change has negative impact on power consumption as
>> the clock stays enabled all the time. This is not what we want... so
>> please explain it more - why you need the clock to be enabled all the
>> time? What is broken (clk_summary is not broken in this case)?
>>
>
> Opps I could not explain some more in my commit message.
>
> Actually TMU sensor for Exynos process are controlled by so external clk
>
> Exynos4412 have VDD18_TS sensor which controls the CLK_SENSE tmu.
> Exynos5422 have VDD18_TS01 / VDD18_TS23 / VDD18_TS4 sensor which
> control the CLK_SENSE tmu.
>
> So as per my understanding tmu is clk driver which control the flow  PMIC.
>
> clk_prepare_enable combine clk_prepare and clk_enable
> and clk_disable_unprepare combine clk_disable and clk_unprepare.
>
> most of the driver prefer clk_prepare_enable and clk_disable_unprepare.
>
> clk_summary is just a reference looking point where we could check the
> clk is enable/disable.
>
> what is broken ?
> I still few more parameter need to tuned to configure the tmu driver.

I am sorry but I am still unable to see what is broken and what are
you trying to fix. I asked what is broken and you replied that there
is a sensor, there is a clock, drivers use clk_prepare_enable and some
more parameter need to be tuned... None of these are answers to
question - what is broken. How can I reproduce the problem?

Best regards,
Krzysztof

Re: [PATCH 1/5] thermal: exynos: enable core tmu clk on exynos platform

[Anand Moon]

[-- Attachment #1: Type: text/plain, Size: 2604 bytes --]

Hi Krzysztof

On 18 July 2018 at 11:47, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 17 July 2018 at 22:23, Anand Moon <linux.amoon@gmail.com> wrote:
>> Hi Krzysztof
>>
>> On 17 July 2018 at 17:50, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> Hi Anand,
>>>
>>> Thanks for patch.
>>>
>>> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>>>> clk_summary do not show tmu_apbif clk enable, so replace
>>>> the clk_prepare with clk_prepare_enables to enable tmu clk.
>>>
>>> This is not valid reason to do a change. What is clk_summary does not
>>> really matter. Your change has negative impact on power consumption as
>>> the clock stays enabled all the time. This is not what we want... so
>>> please explain it more - why you need the clock to be enabled all the
>>> time? What is broken (clk_summary is not broken in this case)?
>>>
>>
>> Opps I could not explain some more in my commit message.
>>
>> Actually TMU sensor for Exynos process are controlled by so external clk
>>
>> Exynos4412 have VDD18_TS sensor which controls the CLK_SENSE tmu.
>> Exynos5422 have VDD18_TS01 / VDD18_TS23 / VDD18_TS4 sensor which
>> control the CLK_SENSE tmu.
>>
>> So as per my understanding tmu is clk driver which control the flow  PMIC.
>>
>> clk_prepare_enable combine clk_prepare and clk_enable
>> and clk_disable_unprepare combine clk_disable and clk_unprepare.
>>
>> most of the driver prefer clk_prepare_enable and clk_disable_unprepare.
>>
>> clk_summary is just a reference looking point where we could check the
>> clk is enable/disable.
>>
>> what is broken ?
>> I still few more parameter need to tuned to configure the tmu driver.
>
> I am sorry but I am still unable to see what is broken and what are
> you trying to fix. I asked what is broken and you replied that there
> is a sensor, there is a clock, drivers use clk_prepare_enable and some
> more parameter need to be tuned... None of these are answers to
> question - what is broken. How can I reproduce the problem?
>
> Best regards,
> Krzysztof

Basically I use thermal testing.

# git clone https://git.linaro.org/power/pm-qa.git
# cd pm-qa
# make -C thermal check

most of the testcase failed on Exynos5422 but some pass on Exynos4412.

Attach is the software overview from Exynos5422 user manual.

I am not able to explain in deep technically, but I have studied other thermal
driver to draw into conclusion that tmu clk's need to be enabled.

If you feel the we should not enable these clk, them I will drop the
clk_prepare_enable check
and resubmit the changes with better commit message.

Best Regards
-Anand

[-- Attachment #2: Exynos5422_software_tmu.txt --]
[-- Type: text/plain, Size: 1582 bytes --]

The example of software sequence as follows:
The parameter may be changed according to an application.
/* Read the measured data from e-fuse */
Triminfo_85 = TRIMINFO[15:8]
Triminfo_25 = TRIMINFO[7:0]
/* Calibrated threshold temperature is written into THRES_TEMP_RISE and THRES_TEMP_FALL */
/* Refer to 1.6.1 */
THRES_TEMP_RISE0 = 0x40;
THRES_TEMP_RISE1 = 0x50;
THRES_TEMP_RISE2 = 0x60;
THRES_TEMP_RISE3 = 0x70;
THRES_TEMP_FALL0 = 0x3A;
THRES_TEMP_FALL1 = 0x4A;
THRES_TEMP_FALL2 = 0x5A;
/* Parameter for sampling interval is set */
SAMPLING_INTERVAL = 0x1;
/* Interrupt enable */
INTEN[24] =0x1; // for INTEN_FALL2
INTEN[20] =0x1; // for INTEN_FALL1
INTEN[16] =0x1; // for INTEN_FALL0
INTEN[8] =0x1; // for INTEN_RISE2
INTEN[4] =0x1; // for INTEN_RISE1
INTEN[0] =0x1; // for INTEN_RISE0
/* Thermal tripping mode selection */
THERM_TRIP_MODE = 0x4;
/* Thermal tripping enable */
THERM_TRIP_EN = 0x1;
/* Check sensing operation is idle */
tmu_idle = 0;
while(tmu_idle&1) {
tmu_idle = TMU_STATUS[0];
}
/* Start sensing operation */
TMU_CONTROL |= 1;

ISR_INTREQ_TMU () {
/* Read interrupt status register */
int_status = INTSTAT;
if(int_status[24]) {
ISR_INT_FALL2();
}
else if(int_status[20]) {
ISR_INT_FALL1();
}
else if(int_status[16]) {
ISR_INT_FALL0();
}
Else if(int_status[8]) {
ISR_INT_RISE2();
}
else if(int_status[4]) {
ISR_INT_RISE1();
}
else if(int_status[0]) {
ISR_INT_RISE0();
}
else {
$display("Some error occurred..!");
}
ISR_INT0 () {
/* Perform proper task for decrease temperature */
INTCLEAR[0] = 0x1;
}

Re: [PATCH 1/5] thermal: exynos: enable core tmu clk on exynos platform

[Krzysztof Kozlowski]

On 18 July 2018 at 11:24, Anand Moon <linux.amoon@gmail.com> wrote:
> Hi Krzysztof
>
> On 18 July 2018 at 11:47, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 17 July 2018 at 22:23, Anand Moon <linux.amoon@gmail.com> wrote:
>>> Hi Krzysztof
>>>
>>> On 17 July 2018 at 17:50, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> Hi Anand,
>>>>
>>>> Thanks for patch.
>>>>
>>>> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>>>>> clk_summary do not show tmu_apbif clk enable, so replace
>>>>> the clk_prepare with clk_prepare_enables to enable tmu clk.
>>>>
>>>> This is not valid reason to do a change. What is clk_summary does not
>>>> really matter. Your change has negative impact on power consumption as
>>>> the clock stays enabled all the time. This is not what we want... so
>>>> please explain it more - why you need the clock to be enabled all the
>>>> time? What is broken (clk_summary is not broken in this case)?
>>>>
>>>
>>> Opps I could not explain some more in my commit message.
>>>
>>> Actually TMU sensor for Exynos process are controlled by so external clk
>>>
>>> Exynos4412 have VDD18_TS sensor which controls the CLK_SENSE tmu.
>>> Exynos5422 have VDD18_TS01 / VDD18_TS23 / VDD18_TS4 sensor which
>>> control the CLK_SENSE tmu.
>>>
>>> So as per my understanding tmu is clk driver which control the flow  PMIC.
>>>
>>> clk_prepare_enable combine clk_prepare and clk_enable
>>> and clk_disable_unprepare combine clk_disable and clk_unprepare.
>>>
>>> most of the driver prefer clk_prepare_enable and clk_disable_unprepare.
>>>
>>> clk_summary is just a reference looking point where we could check the
>>> clk is enable/disable.
>>>
>>> what is broken ?
>>> I still few more parameter need to tuned to configure the tmu driver.
>>
>> I am sorry but I am still unable to see what is broken and what are
>> you trying to fix. I asked what is broken and you replied that there
>> is a sensor, there is a clock, drivers use clk_prepare_enable and some
>> more parameter need to be tuned... None of these are answers to
>> question - what is broken. How can I reproduce the problem?
>>
>> Best regards,
>> Krzysztof
>
> Basically I use thermal testing.
>
> # git clone https://git.linaro.org/power/pm-qa.git
> # cd pm-qa
> # make -C thermal check
>
> most of the testcase failed on Exynos5422 but some pass on Exynos4412.
>
> Attach is the software overview from Exynos5422 user manual.
>
> I am not able to explain in deep technically, but I have studied other thermal
> driver to draw into conclusion that tmu clk's need to be enabled.

That is true in general - the clk has to be enabled in certain cases.
However you did not say at all when you want this clock to be
enabled... and the your patch enables it for entire lifetime of
device.

> If you feel the we should not enable these clk, them I will drop the
> clk_prepare_enable check
> and resubmit the changes with better commit message.

I don't know. This was fifth email in this thread and it is the first
time some real problem is mentioned. Still the issue is not described
entirely so I really do not have a clue whether this patch fixes
something or not.
What is more, you mentioned falling pm-qa tests here (not in commit
msg) but did not say whether this patch fixes anything or not.

So let me summarize it:
1. You did not describe the problem you want to fix.
2. The patch looks incorrect because it enables the clock for entire
lifetime of device which we do not want.
3. The patch might or not might fix some problem. We even do not know what...
4. The clock not being enabled when not needed... is obviously not a problem.

Please start from beginning. Find the problem, tell us how it can be
reproduced and deliver a single patch which fixes the problem.

This pattern of your code - fixing something without describing the
problem - happened many time before. I repeated this some times before
as well. I would prefer not to repeat to many times. Therefore I would
be happy if you follow the path mentioned in paragraph before always:
find the problem, tell how it can be reproduced, deliver single patch
which fixes the problem.

Best regards,
Krzysztof

Re: [PATCH 1/5] thermal: exynos: enable core tmu clk on exynos platform

[Anand Moon]

Hi Krzysztof,

On 18 July 2018 at 15:36, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 18 July 2018 at 11:24, Anand Moon <linux.amoon@gmail.com> wrote:
>> Hi Krzysztof
>>
>> On 18 July 2018 at 11:47, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> On 17 July 2018 at 22:23, Anand Moon <linux.amoon@gmail.com> wrote:
>>>> Hi Krzysztof
>>>>
>>>> On 17 July 2018 at 17:50, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>> Hi Anand,
>>>>>
>>>>> Thanks for patch.
>>>>>
>>>>> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote:
>>>>>> clk_summary do not show tmu_apbif clk enable, so replace
>>>>>> the clk_prepare with clk_prepare_enables to enable tmu clk.
>>>>>
>>>>> This is not valid reason to do a change. What is clk_summary does not
>>>>> really matter. Your change has negative impact on power consumption as
>>>>> the clock stays enabled all the time. This is not what we want... so
>>>>> please explain it more - why you need the clock to be enabled all the
>>>>> time? What is broken (clk_summary is not broken in this case)?
>>>>>
>>>>
>>>> Opps I could not explain some more in my commit message.
>>>>
>>>> Actually TMU sensor for Exynos process are controlled by so external clk
>>>>
>>>> Exynos4412 have VDD18_TS sensor which controls the CLK_SENSE tmu.
>>>> Exynos5422 have VDD18_TS01 / VDD18_TS23 / VDD18_TS4 sensor which
>>>> control the CLK_SENSE tmu.
>>>>
>>>> So as per my understanding tmu is clk driver which control the flow  PMIC.
>>>>
>>>> clk_prepare_enable combine clk_prepare and clk_enable
>>>> and clk_disable_unprepare combine clk_disable and clk_unprepare.
>>>>
>>>> most of the driver prefer clk_prepare_enable and clk_disable_unprepare.
>>>>
>>>> clk_summary is just a reference looking point where we could check the
>>>> clk is enable/disable.
>>>>
>>>> what is broken ?
>>>> I still few more parameter need to tuned to configure the tmu driver.
>>>
>>> I am sorry but I am still unable to see what is broken and what are
>>> you trying to fix. I asked what is broken and you replied that there
>>> is a sensor, there is a clock, drivers use clk_prepare_enable and some
>>> more parameter need to be tuned... None of these are answers to
>>> question - what is broken. How can I reproduce the problem?
>>>
>>> Best regards,
>>> Krzysztof
>>
>> Basically I use thermal testing.
>>
>> # git clone https://git.linaro.org/power/pm-qa.git
>> # cd pm-qa
>> # make -C thermal check
>>
>> most of the testcase failed on Exynos5422 but some pass on Exynos4412.
>>
>> Attach is the software overview from Exynos5422 user manual.
>>
>> I am not able to explain in deep technically, but I have studied other thermal
>> driver to draw into conclusion that tmu clk's need to be enabled.
>
> That is true in general - the clk has to be enabled in certain cases.
> However you did not say at all when you want this clock to be
> enabled... and the your patch enables it for entire lifetime of
> device.
>
>> If you feel the we should not enable these clk, them I will drop the
>> clk_prepare_enable check
>> and resubmit the changes with better commit message.
>
> I don't know. This was fifth email in this thread and it is the first
> time some real problem is mentioned. Still the issue is not described
> entirely so I really do not have a clue whether this patch fixes
> something or not.
> What is more, you mentioned falling pm-qa tests here (not in commit
> msg) but did not say whether this patch fixes anything or not.
>
> So let me summarize it:
> 1. You did not describe the problem you want to fix.
> 2. The patch looks incorrect because it enables the clock for entire
> lifetime of device which we do not want.
> 3. The patch might or not might fix some problem. We even do not know what...
> 4. The clock not being enabled when not needed... is obviously not a problem.
>
> Please start from beginning. Find the problem, tell us how it can be
> reproduced and deliver a single patch which fixes the problem.
>
> This pattern of your code - fixing something without describing the
> problem - happened many time before. I repeated this some times before
> as well. I would prefer not to repeat to many times. Therefore I would
> be happy if you follow the path mentioned in paragraph before always:
> find the problem, tell how it can be reproduced, deliver single patch
> which fixes the problem.
>
> Best regards,
> Krzysztof

Yes I will try to improve my self to the point commit message
and code as per the documentation.

Let me clear my thoughts on clk's turned on for by driver.
Please correct me If I am wrong.

Exynos support Power domain to control the dynamic power consumption via clk.
Operating voltage
Operating frequency
Toggling ratios of the logic gate

PMU generates power control signal to regulator or PMIC.

To reduce the dynamic power consumption, Exynos SCP uses clock gating
and frequency scaling.

Exynos process support tmu power domain
Power Domain         Power Source         Internal Power Gating Method
           Included Modules

TEMP                     VDD18_TEMP        None
                       TEMP SENSOR

Exynos TMU is directly co-related to CPU frequency scaling module.
as their is increase in CPU clk speed with increase in temperature
TMU will control the flow of cpu speed via clk freq scaling.

It might be a good option to implement pm_runtime for tmu driver.

If you are not satisfied with this series of change lets drop them.
I will just re-submit err-clean up and e-Fuse changes.

Best Regards
-Anand

Re: [PATCH 1/5] thermal: exynos: enable core tmu clk on exynos platform

[Eduardo Valentin]

On Thu, Jul 19, 2018 at 03:22:21PM +0530, Anand Moon wrote:
> Hi Krzysztof,
> 


Please send a new version of the series with applied fixes
and also get reviews from the samsung devs. Thanks.

> 
> Best Regards
> -Anand