Re: [PATCH v4 11/24] PM / devfreq: tegra30: Add debug messages

[Chanwoo Choi]

On 19. 7. 19. ìì 10:13, Dmitry Osipenko wrote:
> Ð Thu, 18 Jul 2019 18:07:05 +0900
> Chanwoo Choi <cw00.choi@xxxxxxxxxxx> ÐÐÑÐÑ:
> 
>> On 19. 7. 18. ìì 12:46, Dmitry Osipenko wrote:
>>> 17.07.2019 9:45, Chanwoo Choi ÐÐÑÐÑ:  
>>>> On 19. 7. 16. ìí 10:26, Dmitry Osipenko wrote:  
>>>>> 16.07.2019 15:23, Chanwoo Choi ÐÐÑÐÑ:  
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> Usually, the kernel log print for all users
>>>>>> such as changing the frequency, fail or success.
>>>>>>
>>>>>> But, if the log just show the register dump,
>>>>>> it is not useful for all users. It is just used
>>>>>> for only specific developer.
>>>>>>
>>>>>> I recommend that you better to add more exception handling
>>>>>> code on many points instead of just showing the register dump.  
>>>>>
>>>>> The debug messages are not users, but for developers. Yes, I
>>>>> primarily made the debugging to be useful for myself and will be
>>>>> happy to change the way debugging is done if there will be any
>>>>> other active developer for this driver. The registers dump is
>>>>> more than enough in order to understand what's going on, I don't
>>>>> see any real need to change anything here for now.  
>>>>
>>>> Basically, we have to develop code and add the log for anyone.
>>>> As you commented, even if there are no other developer, we never
>>>> guarantee this assumption forever. And also, if added debug message
>>>> for only you, you can add them when testing it temporarily.
>>>>
>>>> If you want to add the just register dump log for you,
>>>> I can't agree. Once again, I hope that anyone understand
>>>> the meaning of debug message as much possible as.
>>>>  
>>>
>>> The registers dump should be good for everyone because it's a
>>> self-explanatory information for anyone who is familiar with the
>>> hardware. I don't think there is a need for anything else than what
>>> is proposed in this patch, at least for now. I also simply don't
>>> see any other better way to debug the state of this particular
>>> hardware, again this logging is for the driver developers and not
>>> for users.
>>>
>>> Initially, I was temporarily adding the debug messages. Now they are
>>> pretty much mandatory for verifying that driver is working
>>> properly. And of course the debugging messages got into the shape
>>> of this patch after several iterations of refinements. So again, I
>>> suppose that this should be good enough for everyone who is
>>> familiar with the hardware. And of course I'm open to the
>>> constructive suggestions, the debugging aid is not an ABI and could
>>> be changed/improved at any time.
>>>
>>> You're suggesting to break down the debugging into several smaller
>>> pieces, but I'm finding that as not a constructive suggestion
>>> because the information about the full hardware state is actually
>>> necessary for the productive debugging.
>>>
>>>   
>>
>> Sorry for that as I saie, I cannot agree this patch. In my case,
>> I don't understand what is meaning of register dump of this patch.
>> I knew that just register dump are useful for real developer.
> 
> It's not only a registers dump, as you may see there is also a dump of
> other properties like boosting value, OPPs selection and etc.
> 
> It looks to me that you're also missing important detail that debug
> messages are compiled out unless DEBUG is defined for the drivers
> build. So in order to get the debug message a user shall explicitly add
> #define DEBUG macro to the code or enable debug messages globally in
> the kernel's config. There is also an option for dynamic debug messages
> in the kernel, but it doesn't matter now because all these messages are
> turned into tracepoints later in the patch #17.


Right. But, this patch could not the split up between register dump and others.
As I said repeatly, I hope to add the log that anyone can understand. 


> 
>> If you want to show the register dump, you better to add some feature
>> with debugfs for devfreq framework in order to read the register dump.
>> As I knew, sound framework (alsa) has the similar feature for checking
>> the register dump.
>>
> 
> The intent was to have an option for dynamic debugging of the driver and
> initially debug messages were good enough, but then it became not enough
> and hence the debug messages were turned into tracepoints in the patch
> #17. Would it be acceptable to squash this patch and #17?
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics