Re: [PATCH V6 0/7] Add driver support for Data Capture and Compare Engine(DCC) for SM8150,SC7280,SC7180,SDM845

[Souradeep Chowdhury <quic_schowdhu@quicinc.com>]

On 12/16/2021 9:18 PM, Thara Gopinath wrote:
>
>
> On 8/10/21 1:54 PM, Souradeep Chowdhury wrote:
>> DCC(Data Capture and Compare) is a DMA engine designed for debugging 
>> purposes.In case of a system
>> crash or manual software triggers by the user the DCC hardware stores 
>> the value at the register
>> addresses which can be used for debugging purposes.The DCC driver 
>> provides the user with sysfs
>> interface to configure the register addresses.The options that the 
>> DCC hardware provides include
>> reading from registers,writing to registers,first reading and then 
>> writing to registers and looping
>> through the values of the same register.
>>
>> In certain cases a register write needs to be executed for accessing 
>> the rest of the registers,
>> also the user might want to record the changing values of a register 
>> with time for which he has the
>> option to use the loop feature.
>
> Hello Souradeep,
>
> First of all, I think this is very a useful feature to have. I have 
> some generic design related queries/comments on driver and the 
> interface exposed to the user space. Also, I do not understand the h/w 
> well here, so feel free to correct me if I am wrong.
>
> 1. Linked list looks like a very internal feature to the h/w. It 
> really is not an info that user should be aware of. I tried reading 
> the code a bit. IUC, every time a s/w trigger is issued the configs in 
> all the enabled linked lists are executed. The final ram dump that you 
> get from /dev/dcc_sram is a dump of contents from all the enabled 
> list? Is this understanding correct ? And we are talking of at-most 4 
> linked list?
> If yes, I think it might be better to have a folder per linked list 
> with config, config_write etc. Also if possible it will be better to 
> dump the results to a file in the specific folder instead of reading 
> from /dev/dcc_sram.
> If no, there is no real need for user to know the linked list, right? 
> Choosing of linked list can be done by kernel driver in this case with 
> no input needed from user.
>
> 2. Now to the sysfs interface itself, I know lot of thought has gone 
> into sysfs vs debugfs considerations. But, have you considered using 
> netlink interface instead of sysfs. Netlink interface is used for 
> asynchronous communication between kernel and user space. In case of 
> DCC, the communication appears to be asynchronous, where in user asks 
> the kernel to capture some info and kernel can indicate back to user 
> when the info is captured. Also the entire mess surrounding echoing 
> addr / value / offset repeatedly into a sysfs entry can be avoided 
> using netlink interface.
>
Hello Thara,

Thanks for your review comments. Following are some points from my end


1) Each linked list represent a particular block of memory in DCC_SRAM 
which is preserved for that particular list. That is why offset 
calculation is done on the driver based on the linked list chosen by the 
user.

     This choice needs to be made by the user since the number for the 
linked list chosen is specific to the registers used to debug a 
particular component.  Also we are giving the user flexibility to 
configure multiple

     linked lists at one go so that even if we don't have a separate 
folder for it , the dumps are collected as a separate list of registers. 
Also there are certain curr_list values which may be supported by the dcc

     hardware but may not be accessible to the user and so the choice 
cannot be made arbitrarily from the driver.


2) From opensource, I can see that Netlink has been used in most of the 
cases where we need to notify stats to the user by taking the advantage 
of asynchronous communication. In this case, that requirement is not

     there since it is mostly one way communication from user to kernel. 
Also since this is used for debugging purposes perhaps sysfs adds more 
reliability than Netlink. In case of Netlink we have the additional

      overhead of dealing with socket calls. Let me know otherwise.


Thanks,

Souradeep