Re: [PATCH V1 2/6] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC)

From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Cc: Souradeep Chowdhury <schowdhu@codeaurora.org>,
	Rob Herring <robh+dt@kernel.org>, Andy Gross <agross@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	devicetree@vger.kernel.org, sibis@codeaurora.org,
	Rajendra Nayak <rnayak@codeaurora.org>,
	vkoul@kernel.org
Subject: Re: [PATCH V1 2/6] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC)
Date: Thu, 11 Mar 2021 10:31:16 -0600	[thread overview]
Message-ID: <YEpF1JO4CAd2pb0m@builder.lan> (raw)
In-Reply-To: <ab30490c016f906fd9bc5d789198530b@codeaurora.org>

On Thu 11 Mar 00:19 CST 2021, Sai Prakash Ranjan wrote:

> Hi Bjorn,
> 
> On 2021-03-11 04:49, Bjorn Andersson wrote:
> > On Wed 10 Mar 10:46 CST 2021, Souradeep Chowdhury wrote:
> > 
> > > The DCC is a DMA Engine designed to capture and store data
> > > during system crash or software triggers. The DCC operates
> > > based on link list entries which provides it with data and
> > > addresses and the function it needs to perform. These
> > > functions are read, write and loop. Added the basic driver
> > > in this patch which contains a probe method which instantiates
> > > the resources needed by the driver. DCC has it's own SRAM which
> > > needs to be instantiated at probe time as well.
> > > 
> > 
> > So to summarize, the DCC will upon a crash copy the configured region
> > into the dcc-ram, where it can be retrieved either by dumping the memory
> > over USB or from sysfs on the next boot?
> > 
> 
> Not just the next boot, but also for the current boot via /dev/dcc_sram,
> more below.
> 

Interesting!

> > > Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org>
> > > ---
> > >  drivers/soc/qcom/Kconfig  |   8 +
> > >  drivers/soc/qcom/Makefile |   1 +
> > >  drivers/soc/qcom/dcc.c    | 388
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 397 insertions(+)
> > >  create mode 100644 drivers/soc/qcom/dcc.c
> > > 
> 
> <snip>...
> 
> > 
> > How about implementing this using pstore instead of exposing it through
> > a custom /dev/dcc_sram (if I read the code correclty)
> > 
> 
> Using pstore is definitely a good suggestion, we have been thinking of
> adding it as a separate change once the basic support for DCC gets in.
> But pstore ram backend again depends on warm reboot which is present only
> in chrome compute platforms but android platforms do not officially support
> warm reboot. Pstore with block devices as a backend would be ideal but it
> is still a work in progress to use mmc as the backend [1].
> 

I was under the impression that we can have multiple pstore
implementations active, so we would have ramoops and dcc presented
side by side after such restart. Perhaps that's a misunderstanding on my
part?

> Now the other reason as to why we need this dev interface in addition to
> pstore,
> 
>  * Pstore contents are retrieved on the next boot, but DCC SRAM contents
>    can be collected via dev interface for the current boot via SW trigger.
>    Lets say we have some non-fatal errors in one of the subsystems and we
>    want to analyze the register values, it becomes as simple as configuring
>    that region, trigger dcc and collect the sram contents and parse it.
> 
>    echo "addr" > /sys/bus/platform/devices/***.dcc/config
>    echo  1 > /sys/bus/platform/devices/***.dcc/trigger
>    cat /dev/dcc_sram > dcc_sram.bin
>    python dcc_parser.py -s dcc_sram.bin --v2 -o output/
> 
> We don't have to reboot at all for SW triggers. This is very useful and
> widely used internally.
> 
> Is the custom /dev/dcc_sram not recommended because of the dependency on
> the userspace component being not available openly? If so, we already have
> the dcc parser upstream which we use to extract the sram contents [2].
> 

My concern is that we come up with a custom chardev implementation for
something that already exists or should exist in a more generic form.

In the runtime sequence above, why don't you have trigger just generate
a devcoredump? But perhaps the answer is that we want a unified
interface between the restart and runtime use cases?

It would be nice to have some more details of how I can use this and how
to operate the sysfs interface to achieve that, would you be able to
elaborate on this?

Regards,
Bjorn

> [1]
> https://lore.kernel.org/lkml/20210120121047.2601-1-bbudiredla@marvell.com/
> [2] https://source.codeaurora.org/quic/la/platform/vendor/qcom-opensource/tools/tree/dcc_parser
> 
> Thanks,
> Sai
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation