Re: [PATCH 1/2] thermal: exynos: Reorder exynos_map_dt_data() function

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Joonyoung,

> Hi Lukasz,
> 
> On 04/17/2015 09:39 PM, Lukasz Majewski wrote:
> > Hi Joonyoung,
> > 
> >> Hi Lukasz,
> >>
> >> On 04/15/2015 08:05 PM, Lukasz Majewski wrote:
> >>> Hi Joonyoung,
> >>>
> >>>> Hi Lukasz,
> >>>>
> >>>> On 01/30/2015 05:14 PM, Lukasz Majewski wrote:
> >>>>> Hi Eduardo, Abhilash,
> >>>>>
> >>>>>> On Thu, Jan 22, 2015 at 06:02:07PM +0530, Abhilash Kesavan
> >>>>>> wrote:
> >>>>>>> Hi Lukasz,
> >>>>>>>
> >>>>>>> On Thu, Jan 22, 2015 at 2:31 PM, Lukasz Majewski
> >>>>>>> <l.majewski@samsung.com> wrote:
> >>>>>>>> Hi Abhilash,
> >>>>>>>>
> >>>>>>>>> Hi Lukasz,
> >>>>>>>>>
> >>>>>>>>> On Mon, Jan 19, 2015 at 5:14 PM, Lukasz Majewski
> >>>>>>>>> <l.majewski@samsung.com> wrote:
> >>>>>>>>>> The exynos_map_dt_data() function must be called before
> >>>>>>>>>> thermal_zone_of_sensor_register(), and hence provide
> >>>>>>>>>> tmu_read() function, before it is needed.
> >>>>>>>>>>
> >>>>>>>>>> This change is driven by adding support for enabling
> >>>>>>>>>> thermal_zoneX when it is properly initialized.
> >>>>>>>>>>
> >>>>>>>>>> One can read the mode of operation
> >>>>>>>>>> at /sys/class/thermal/thermal_zone0/mode Such functionality
> >>>>>>>>>> was missing in the of-thermal.c code.
> >>>>>>>>>>
> >>>>>>>>>> Reported-by: Abhilash Kesavan <a.kesavan@samsung.com>
> >>>>>>>>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  drivers/thermal/samsung/exynos_tmu.c | 7 ++++---
> >>>>>>>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c
> >>>>>>>>>> b/drivers/thermal/samsung/exynos_tmu.c index
> >>>>>>>>>> 9d2d685..5d946ab 100644 ---
> >>>>>>>>>> a/drivers/thermal/samsung/exynos_tmu.c +++
> >>>>>>>>>> b/drivers/thermal/samsung/exynos_tmu.c @@ -975,15 +975,16
> >>>>>>>>>> @@ static int exynos_tmu_probe(struct platform_device
> >>>>>>>>>> *pdev) platform_set_drvdata(pdev, data);
> >>>>>>>>>> mutex_init(&data->lock);
> >>>>>>>>>>
> >>>>>>>>>> +       ret = exynos_map_dt_data(pdev);
> >>>>>>>>>> +       if (ret)
> >>>>>>>>>> +               goto err_sensor;
> >>>>
> >>>> It's enough to just return ret.
> >>>>
> >>>> One more, i think to need to take out regulator enable codes from
> >>>> exynos_map_dt_data. If not, can't restore about regulator when
> >>>> error occurs.
> >>>>
> >>>>>>>>>> +
> >>>>>>>>>>         data->tzd =
> >>>>>>>>>> thermal_zone_of_sensor_register(&pdev->dev, 0, data,
> >>>>>>>>>> &exynos_sensor_ops); if (IS_ERR(data->tzd)) {
> >>>>>>>>>>                 pr_err("thermal: tz: %p ERROR\n",
> >>>>>>>>>> data->tzd); return PTR_ERR(data->tzd);
> >>>>>>>>>>         }
> >>>>>>>>>> -       ret = exynos_map_dt_data(pdev);
> >>>>>>>>>> -       if (ret)
> >>>>>>>>>> -               goto err_sensor;
> >>>>>>>>>>
> >>>>>>>>>>         pdata = data->pdata;
> >>>>>>>>>
> >>>>>>>>> I have been testing this along with your v5 patch set and am
> >>>>>>>>> seeing incorrect temperature being reported at boot-up on
> >>>>>>>>> exynos7.
> >>>>>>>>
> >>>>>>>> Does it show a maximal temperature value (0x1FF)?
> >>>>>>>
> >>>>>>> I did not print the current temperature register, but I
> >>>>>>> remember the message showing ~105C. Will give you the
> >>>>>>> register value when I test with more debug prints tomorrow.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>>  It looks
> >>>>>>>>> like exynos_tmu_read gets called from
> >>>>>>>>> thermal_zone_of_device_update during boot-up, now that we
> >>>>>>>>> have it populated early. However, as the tmu initialization
> >>>>>>>>> function has not been called yet it returns a wrong value.
> >>>>>>>>> Does that sound correct ?
> >>>>>>>>
> >>>>>>>> No, this is a mistake. However, I'm wondering why on
> >>>>>>>> Exynos4/5 this error didn't show up...
> >>>>>>>
> >>>>>>> I have been lowering the software trip point temperature in
> >>>>>>> the exynos7 dts file (to 55C) for testing purposes. Hence,
> >>>>>>> when the temperature is read incorrectly as ~105C the board
> >>>>>>> trips at boot-up
> >>>>>
> >>>>> 					^^^^ this is a very
> >>>>> unusual value - I had problems with
> >>>>> 					reading 0xFF values with
> >>>>> 					similar symptom (but
> >>>>> this was caused by lack of vtmu).
> >>>>>
> >>>>>>> itself. Maybe for exynos4/5 the incorrect value read during
> >>>>>>> boot-up is in the non-tripping range and once the tmu is
> >>>>>>> initialized later it continues to function properly
> >>>>>>> thereafter ?
> >>>>>>>
> >>>>>>>>
> >>>>>>>> The reordering is needed to be able to call set_mode callback
> >>>>>>>> at of-thermal.c to set the mode.
> >>>>>>>>
> >>>>>>>> If this change causes problems, then another solution
> >>>>>>>> (probably not so neat) must be found.
> >>>>>>>
> >>>>>>> Please let me know if you need any further details.
> >>>>>
> >>>>> Abhilash, could you provide more details (like relevant output
> >>>>> from dmesg) and point me a list of patches which shall I apply
> >>>>> to test this issue on Exynos4/5?
> >>>>>
> >>>>>>
> >>>>>> What is the status of this patch? Is it still required?
> >>>>>
> >>>>> It is strange, since on Exynos4/5 this works and some problems
> >>>>> show up when run on Exynos7.
> >>>>>
> >>>>
> >>>> I'm also wondering the status of this patch.
> >>>
> >>> This patch has been dropped.
> >>>
> >>>>
> >>>> I get below errors when boots odroidxu3 board without this patch.
> >>>
> >>> Could you share the exact SHA1 and branch which you use in your
> >>> setup?
> >>>
> >>
> >> I tested with of odroidxu3 patchset for pwm-fan control of Anand
> >> Moon on 20150414 -next.
> >>
> >> http://www.spinics.net/lists/arm-kernel/msg412031.html
> >>
> >>> For a reference please check following branch at github (this is
> >>> the code which should be merged to v4.1-rc1)
> >>>
> >>> git@github.com:lmajewski/linux-samsung-thermal.git
> >>>
> >>> branch: next [1]
> >>>
> >>> This branch includes exynos7 support done by Chanwoo.
> >>>
> >>
> >> I got following errors from branch [1] without patchset of Anand
> >> Moon,
> >>
> >> [    4.576442] thermal thermal_zone0: failed to read out thermal
> >> zone 0 [    4.581685] 10060000.tmu supply vtmu not found, using
> >> dummy regulator [    4.588223] thermal thermal_zone1: failed to
> >> read out thermal zone 1 [    4.594110] 10064000.tmu supply vtmu
> >> not found, using dummy regulator [    4.600849] thermal
> >> thermal_zone2: failed to read out thermal zone 2 [    4.606847]
> >> 10068000.tmu supply vtmu not found, using dummy regulator
> >> [    4.613640] thermal thermal_zone3: failed to read out thermal
> >> zone 3 [    4.619584] 1006c000.tmu supply vtmu not found, using
> >> dummy regulator [    4.626370] thermal thermal_zone4: failed to
> >> read out thermal zone 4 [    4.632324] 100a0000.tmu supply vtmu
> >> not found, using dummy regulator
> > 
> > Could you check if after booting you can read temperature from
> > thermal zones?
> > 
> 
> Yes, i can read temperature from
> /sys/devices/virtual/thermal/thermal_zone[X]/temp after booting.

I'm aware of "thermal thermal_zone4: failed to read out thermal
zone" messages which show up during booting.

It is to prevent crashing TMU when it is not yet properly configured.
The problem is related with dependency of of-thermal and exynos
specific code.

I'm planning to fix this issue.

> 
> Thanks.
> 
> >>
> >>>>
> >>>> [    4.831980] thermal thermal_zone0: failed to read out thermal
> >>>> zone (-22) [    4.838096] thermal thermal_zone1: failed to read
> >>>> out thermal zone (-22) [    4.844894] thermal thermal_zone2:
> >>>> failed to read out thermal zone (-22) [    4.851470] thermal
> >>>> thermal_zone3: failed to read out thermal zone (-22)
> >>>> [    4.858186] thermal thermal_zone4: failed to read out thermal
> >>>> zone (-22)
> >>>>
> >>>
> >>> This issue has been fixed by following patch:
> >>> "thermal: exynos: fix: Check if data->tmu_read callback is present
> >>> before read"
> >>>
> >>> SHA1: 4531fa1684bb883ee01f1a182900b1e15d461b3
> >>>
> >>
> >> I already have this commit on my test branch.
> >>
> >>> Please check [1] if it solves your problems.
> >>>
> >>>> Thanks.
> >>>
> >>
> >> Thanks.
> > 
> > 
> > 
> 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group