Date:   Fri, 7 Jan 2022 08:03:41 -0800
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Souradeep Chowdhury <quic_schowdhu@quicinc.com>
Cc:     Thara Gopinath <thara.gopinath@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
        Sibi Sankar <sibis@codeaurora.org>,
        Rajendra Nayak <rnayak@codeaurora.org>, vkoul@kernel.org
Subject: Re: [PATCH V6 0/7] Add driver support for Data Capture and Compare
 Engine(DCC) for SM8150,SC7280,SC7180,SDM845
Message-ID: <YdhkXRxUQGRRZEsx@ripper>
References: <cover.1628617260.git.schowdhu@codeaurora.org>
 <396edd95-4f38-6830-99da-11e73d62a0cf@linaro.org>
 <705c280b-bced-476d-8e21-1a5afbf3d2f3@quicinc.com>
 <YdeDtlmPRQx3FU9i@ripper>
 <84d36c7f-d75e-61f9-7670-c651cc50d083@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <84d36c7f-d75e-61f9-7670-c651cc50d083@quicinc.com>
Precedence: bulk

On Fri 07 Jan 07:43 PST 2022, Souradeep Chowdhury wrote:

> 
> 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.
> 

Makes sense. But I think you should use "list" (or "sequence") and not
"linked list" in the API/documentation then.

> ????????????? /* 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.
> 

But as the DCC supports reading, writing, looping and rmw I don't think
it's correct to say that a list is a "group of register values". It's a
"sequence (or list) of operations".

Regards,
Bjorn

> > 
> > 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
> > > 
> > > 
> > > 
> > > 
> > >