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

Stephen Boyd <swboyd@xxxxxxxxxxxx> · Mon, 29 Mar 2021 13:05:45 -0700

Quoting Souradeep Chowdhury (2021-03-25 01:02:33)
> The DCC is a DMA Engine designed to capture and store data
> during system crash or software triggers.The DCC operates
> based on user inputs via the sysfs interface.The user gives
> addresses as inputs and these addresses are stored in the
> form of linkedlists.In case of a system crash or a manual
> software trigger by the user through the sysfs interface,
> the dcc captures and stores the values at these addresses.
> This patch contains the driver which has all the methods
> pertaining to the sysfs interface, auxiliary functions to
> support all the four fundamental operations of dcc namely
> read, write, first read then write and loop.The probe method
> here instantiates all the resources necessary for dcc to
> operate mainly the dedicated dcc sram where it stores the
> values.The DCC driver can be used for debugging purposes
> without going for a reboot since it can perform manual
> triggers.
> 
> Signed-off-by: Souradeep Chowdhury <schowdhu@xxxxxxxxxxxxxx>
> ---
>  drivers/soc/qcom/Kconfig  |    8 +
>  drivers/soc/qcom/Makefile |    1 +
>  drivers/soc/qcom/dcc.c    | 1549 +++++++++++++++++++++++++++++++++++++++++++++

Where's the document for various sysfs attributes in Documentation/ABI?

>  3 files changed, 1558 insertions(+)
>  create mode 100644 drivers/soc/qcom/dcc.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 79b568f..8819e0b 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -69,6 +69,14 @@ config QCOM_LLCC
>           SDM845. This provides interfaces to clients that use the LLCC.
>           Say yes here to enable LLCC slice driver.
> 
> +config QCOM_DCC
> +       tristate "Qualcomm Technologies, Inc. Data Capture and Compare engine driver"

Put (DCC) after Compare?

> +       depends on ARCH_QCOM || COMPILE_TEST
> +       help
> +         This option enables driver for Data Capture and Compare engine. DCC
> +         driver provides interface to configure DCC block and read back
> +         captured data from DCC's internal SRAM.
> +
>  config QCOM_KRYO_L2_ACCESSORS
>         bool
>         depends on ARCH_QCOM && ARM64 || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index ad675a6..1b00870 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -26,3 +26,4 @@ obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
>  obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>  obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=        kryo-l2-accessors.o
> +obj-$(CONFIG_QCOM_DCC) += dcc.o

Can this be sorted based on config or file name instead of adding to the
end of the file and leading to endless conflicts?

> diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c
> new file mode 100644
> index 0000000..a55d8ca7
> --- /dev/null
> +++ b/drivers/soc/qcom/dcc.c
> @@ -0,0 +1,1549 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cdev.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +
> +#define TIMEOUT_US             100
> +
> +#define dcc_writel(drvdata, val, off)                                  \
> +       writel((val), drvdata->base + dcc_offset_conv(drvdata, off))
> +#define dcc_readl(drvdata, off)                                                \
> +       readl(drvdata->base + dcc_offset_conv(drvdata, off))
> +
> +#define dcc_sram_readl(drvdata, off)                                   \
> +       readl(drvdata->ram_base + off)
> +
> +#define DCC_SRAM_NODE "dcc_sram"
> +
> +/* DCC registers */
> +#define DCC_HW_INFO                    0x04
> +#define DCC_LL_NUM_INFO                        0x10
> +#define DCC_STATUS                     0x1C
> +#define DCC_LL_LOCK(m)                 (0x34 + 0x80 * m)
> +#define DCC_LL_CFG(m)                  (0x38 + 0x80 * m)
> +#define DCC_LL_BASE(m)                 (0x3c + 0x80 * m)
> +#define DCC_FD_BASE(m)                 (0x40 + 0x80 * m)
> +#define DCC_LL_TIMEOUT(m)              (0x44 + 0x80 * m)
> +#define DCC_LL_INT_ENABLE(m)           (0x4C + 0x80 * m)
> +#define DCC_LL_INT_STATUS(m)           (0x50 + 0x80 * m)
> +#define DCC_LL_SW_TRIGGER(m)           (0x60 + 0x80 * m)
> +#define DCC_LL_BUS_ACCESS_STATUS(m)    (0x64 + 0x80 * m)
> +
> +#define DCC_MAP_LEVEL1                 0x18
> +#define DCC_MAP_LEVEL2                 0x34
> +#define DCC_MAP_LEVEL3                 0x4C
> +
> +#define DCC_MAP_OFFSET1                        0x10
> +#define DCC_MAP_OFFSET2                        0x18
> +#define DCC_MAP_OFFSET3                        0x1C
> +#define DCC_MAP_OFFSET4                        0x8
> +
> +#define DCC_FIX_LOOP_OFFSET            16
> +#define DCC_VER_INFO_BIT               9
> +
> +#define DCC_READ                       0
> +#define DCC_WRITE                      1
> +#define DCC_LOOP                       2
> +#define DCC_READ_WRITE                 3
> +
> +#define MAX_DCC_OFFSET                 GENMASK(9, 2)
> +#define MAX_DCC_LEN                    GENMASK(6, 0)
> +#define MAX_LOOP_CNT                   GENMASK(7, 0)
> +
> +#define DCC_ADDR_DESCRIPTOR            0x00
> +#define DCC_LOOP_DESCRIPTOR            BIT(30)
> +#define DCC_RD_MOD_WR_DESCRIPTOR       BIT(31)
> +#define DCC_LINK_DESCRIPTOR            GENMASK(31, 30)
> +
> +#define DCC_READ_IND                   0x00
> +#define DCC_WRITE_IND                  (BIT(28))
> +
> +#define DCC_AHB_IND                    0x00
> +#define DCC_APB_IND                    BIT(29)
> +
> +#define DCC_MAX_LINK_LIST              8
> +#define DCC_INVALID_LINK_LIST          GENMASK(7, 0)
> +
> +#define DCC_VER_MASK1                  GENMASK(6, 0)
> +#define DCC_VER_MASK2                  GENMASK(5, 0)
> +
> +#define DCC_RD_MOD_WR_ADDR              0xC105E
> +
> +struct qcom_dcc_config {
> +       const int dcc_ram_offset;

Why const here? The container structure can be const and then this is
easier to read.

> +};
> +
> +enum dcc_descriptor_type {
> +       DCC_ADDR_TYPE,
> +       DCC_LOOP_TYPE,
> +       DCC_READ_WRITE_TYPE,
> +       DCC_WRITE_TYPE
> +};
> +
> +enum dcc_mem_map_ver {
> +       DCC_MEM_MAP_VER1 = 1,
> +       DCC_MEM_MAP_VER2 = 2,
> +       DCC_MEM_MAP_VER3 = 3
> +};
> +
> +struct dcc_config_entry {
> +       u32                             base;
> +       u32                             offset;
> +       u32                             len;
> +       u32                             index;
> +       u32                             loop_cnt;
> +       u32                             write_val;
> +       u32                             mask;
> +       bool                            apb_bus;
> +       enum dcc_descriptor_type        desc_type;
> +       struct list_head                list;
> +};
> +
> +struct dcc_drvdata {

Can we get some kernel doc on this structure?

> +       void __iomem            *base;
> +       u32                     reg_size;
> +       struct device           *dev;
> +       struct mutex            mutex;

In particular what this mutex is protecting.

> +       void __iomem            *ram_base;
> +       u32                     ram_size;
> +       u32                     ram_offset;
> +       enum dcc_mem_map_ver    mem_map_ver;
> +       u32                     ram_cfg;
> +       u32                     ram_start;

phys_addr_t?

> +       bool                    *enable;
> +       bool                    *configured;
> +       bool                    interrupt_disable;
> +       char                    *sram_node;

This is always a define, so why make a struct member?

> +       struct cdev             sram_dev;
> +       struct class            *sram_class;
> +       struct list_head        *cfg_head;
> +       u32                     *nr_config;
> +       u32                     nr_link_list;

size_t?

> +       u8                      curr_list;
> +       u8                      loopoff;
> +};
> +
> +struct dcc_cfg_attr {
> +       u32     addr;
> +       u32     prev_addr;
> +       u32     prev_off;
> +       u32     link;
> +       u32     sram_offset;
> +};
> +
> +struct dcc_cfg_loop_attr {
> +       u32     loop;
> +       bool    loop_start;
> +       u32     loop_cnt;
> +       u32     loop_len;
> +       u32     loop_off;
> +};
> +
> +static size_t dcc_offset_conv(struct dcc_drvdata *drvdata, size_t off)
> +{
> +       if (drvdata->mem_map_ver == DCC_MEM_MAP_VER1) {
> +               if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL3)
> +                       return (off - DCC_MAP_OFFSET3);