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

Bjorn Andersson
2021-03-11T16:32:14Z

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