Message-ID: <84d36c7f-d75e-61f9-7670-c651cc50d083@quicinc.com>
Date:   Fri, 7 Jan 2022 21:13:16 +0530
From:   Souradeep Chowdhury <quic_schowdhu@...cinc.com>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
CC:     Thara Gopinath <thara.gopinath@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        "Sai Prakash Ranjan" <saiprakash.ranjan@...eaurora.org>,
        Sibi Sankar <sibis@...eaurora.org>,
        Rajendra Nayak <rnayak@...eaurora.org>, <vkoul@...nel.org>
Subject: Re: [PATCH V6 0/7] Add driver support for Data Capture and Compare
 Engine(DCC) for SM8150,SC7280,SC7180,SDM845


On 1/7/2022 5:35 AM, Bjorn Andersson wrote:
> On Thu 06 Jan 07:20 PST 2022, Souradeep Chowdhury wrote:
>
>> 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.
>>
> But in the end, as you write out the SRAM content, is there really any
> linked lists? Afaict it's just a sequence of operations/commands. The
> linked list part seems to be your data structure of choice to keep track
> of these operations in the driver before flushing them out.

That is correct, the linked list defined in the driver is for storing 
the addresses sequentially in DCC_SRAM and is just an internal

data structure of the driver. However, there is also a "list" from DCC 
hardware perspective. The following driver code shows how

a list is initiated with the beginning and end sram offset so that DCC 
hardware can treat it as a separate list of addresses and dump

the values separately.

               /* 1. Take ownership of the list */
                 dcc_writel(drvdata, BIT(0), DCC_LL_LOCK(list));

                 /* 2. Program linked-list in the SRAM */
                 ram_cfg_base = drvdata->ram_cfg;
                 ret = __dcc_ll_cfg(drvdata, list);
                 if (ret) {
                         dcc_writel(drvdata, 0, DCC_LL_LOCK(list));
                         goto err;
                 }

                 /* 3. program DCC_RAM_CFG reg */
                 dcc_writel(drvdata, ram_cfg_base +
                         drvdata->ram_offset/4, DCC_LL_BASE(list));
                 dcc_writel(drvdata, drvdata->ram_start +
                         drvdata->ram_offset/4, DCC_FD_BASE(list));
                 dcc_writel(drvdata, 0xFFF, DCC_LL_TIMEOUT(list));

                 /* 4. Clears interrupt status register */
                 dcc_writel(drvdata, 0, DCC_LL_INT_ENABLE(list));
                 dcc_writel(drvdata, (BIT(0) | BIT(1) | BIT(2)),
                                         DCC_LL_INT_STATUS(list));

                 drvdata->enable[list] = true;

So when user enters multiple lists, the DCC hardware will process it as 
separate group of register values.

>
> Regards,
> Bjorn
>
>> 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
>>
>>
>>
>>
>>

Powered by blists - more mailing lists