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

From: Vinod Koul
Date: Thu Feb 18 2021 - 02:51:01 EST

Next message: Lizhi Hou: "[PATCH V3 XRT Alveo 10/18] fpga: xrt: VSEC platform driver"
Previous message: Stephen Rothwell: "linux-next: build failure after merge of the gpio-brgl tree"
In reply to: kernel test robot: "Re: [PATCH V0 3/6] soc: qcom: dcc:Add driver support for Data Capture and Compare unit(DCC)"
Next in thread: schowdhu: "Re: [PATCH V0 3/6] soc: qcom: dcc:Add driver support for Data Capture and Compare unit(DCC)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 17-02-21, 12:18, Souradeep Chowdhury wrote:
> The DCC is a DMA Engine designed to capture and store data
> during system crash or software triggers.The DCC operates
^^^
Space after . (quite a few here, pls fix them)

> 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 all the link
> list data specific to a SoC.Methods have also been added to
> handle all the functionalities specific to a linked list.Each
> DCC has it's own SRAM which needs to be instantiated at probe
> time as well.

So help me understand, in case of system crash how will this be used..?

>
> Signed-off-by: Souradeep Chowdhury <schowdhu@xxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/Kconfig | 8 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/dcc.c | 1055 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1064 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"
> + 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
> diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c
> new file mode 100644
> index 0000000..d67452b
> --- /dev/null
> +++ b/drivers/soc/qcom/dcc.c
> @@ -0,0 +1,1055 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#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 BM(lsb, msb) ((BIT(msb) - BIT(lsb)) + BIT(msb))
> +#define BMVAL(val, lsb, msb) ((val & BM(lsb, msb)) >> lsb)
> +#define BVAL(val, n) ((val & BIT(n)) >> n)

Pls use macros available in bitfield.h rather than inventing your own..

> +
> +#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)
> +
> +/* 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 (0xFF * 4)
> +#define MAX_DCC_LEN 0x7F
> +#define MAX_LOOP_CNT 0xFF
> +
> +#define DCC_ADDR_DESCRIPTOR 0x00
> +#define DCC_LOOP_DESCRIPTOR (BIT(30))
> +#define DCC_RD_MOD_WR_DESCRIPTOR (BIT(31))
> +#define DCC_LINK_DESCRIPTOR (BIT(31) | BIT(30))

we have GENMASK() for this

> +
> +#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 0xFF
> +
> +#define DCC_VER_MASK1 0x7F
> +#define DCC_VER_MASK2 0x3F

Genmask for these too...

> +
> +#define DCC_RD_MOD_WR_ADDR 0xC105E
> +
> +struct qcom_dcc_config {
> + const int dcc_ram_offset;
> +};
> +
> +static const struct qcom_dcc_config sm8150_cfg = {
> + .dcc_ram_offset = 0x5000,
> +};

maybe move it down near compatible table?

> +
> +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,
> + DCC_MEM_MAP_VER2,
> + DCC_MEM_MAP_VER3
> +};
> +
> +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 {
> + void __iomem *base;
> + u32 reg_size;
> + struct device *dev;
> + struct mutex mutex;
> + void __iomem *ram_base;
> + u32 ram_size;
> + u32 ram_offset;
> + enum dcc_mem_map_ver mem_map_ver;
> + u32 ram_cfg;
> + u32 ram_start;
> + bool *enable;
> + bool *configured;
> + bool interrupt_disable;
> + char *sram_node;
> + struct cdev sram_dev;
> + struct class *sram_class;
> + struct list_head *cfg_head;
> + u32 *nr_config;
> + u32 nr_link_list;
> + 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 u32 dcc_offset_conv(struct dcc_drvdata *drvdata, u32 off)
> +{
> + if (drvdata->mem_map_ver == DCC_MEM_MAP_VER1) {
> + if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL3)
> + return (off - DCC_MAP_OFFSET3);
> + if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL2)
> + return (off - DCC_MAP_OFFSET2);
> + else if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL1)
> + return (off - DCC_MAP_OFFSET1);
> + } else if (drvdata->mem_map_ver == DCC_MEM_MAP_VER2) {
> + if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL2)
> + return (off - DCC_MAP_OFFSET4);
> + }
> + return off;
> +}
> +
> +static int dcc_sram_writel(struct dcc_drvdata *drvdata,
> + u32 val, u32 off)
> +{
> + if (unlikely(off > (drvdata->ram_size - 4)))
> + return -EINVAL;
> +
> + writel((val), drvdata->ram_base + off);
> +
> + return 0;
> +}
> +
> +static int _dcc_ll_cfg_read_write(struct dcc_drvdata *drvdata,
> +struct dcc_config_entry *entry, struct dcc_cfg_attr *cfg)

this looks a bit hard to read, can you make it better (also you can go
upto 100 chars now), do you checkpatch with --strict option to get
better alignment of code

> +{
> + int ret = 0;

Superfluous init?

> +
> + if (cfg->link) {
> + /* write new offset = 1 to continue
> + * processing the list

kernel uses:
/*
* this is a
* multi line comment style
*/

> + */
> +
> + ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> + if (ret)
> + return ret;
> + cfg->sram_offset += 4;
> + /* Reset link and prev_off */
> + cfg->addr = 0x00;
> + cfg->link = 0;
> + cfg->prev_off = 0;

memset cfg first?

> + cfg->prev_addr = cfg->addr;
> + }
> +
> + cfg->addr = DCC_RD_MOD_WR_DESCRIPTOR;
> +
> + ret = dcc_sram_writel(drvdata, cfg->addr, cfg->sram_offset);
> +

drop this empty line

> + if (ret)
> + return ret;
> +
> + cfg->sram_offset += 4;
> +
> + ret = dcc_sram_writel(drvdata, entry->mask, cfg->sram_offset);
> +
> + if (ret)
> + return ret;
> +
> + cfg->sram_offset += 4;
> +
> + ret = dcc_sram_writel(drvdata, entry->write_val, cfg->sram_offset);
> +
> + if (ret)
> + return ret;
> +
> + cfg->sram_offset += 4;
> +
> + cfg->addr = 0;
> +
> + return ret;
> +}
> +
> +static int _dcc_ll_cfg_loop(struct dcc_drvdata *drvdata, struct dcc_config_entry *entry,
> +struct dcc_cfg_attr *cfg, struct dcc_cfg_loop_attr *cfg_loop, u32 *total_len)

here as well

> +{
> +
> + int ret = 0;
> +
> + /* Check if we need to write link of prev entry */
> + if (cfg->link) {
> + ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> + if (ret)
> + return ret;
> + cfg->sram_offset += 4;
> + }
> +
> + if (cfg_loop->loop_start) {
> + cfg_loop->loop = (cfg->sram_offset - cfg_loop->loop_off) / 4;
> + cfg_loop->loop |= (cfg_loop->loop_cnt << drvdata->loopoff) &
> + BM(drvdata->loopoff, 27);
> + cfg_loop->loop |= DCC_LOOP_DESCRIPTOR;
> + *total_len += (*total_len - cfg_loop->loop_len) * cfg_loop->loop_cnt;
> +
> + ret = dcc_sram_writel(drvdata, cfg_loop->loop, cfg->sram_offset);
> +
> + if (ret)
> + return ret;
> + cfg->sram_offset += 4;
> +
> + cfg_loop->loop_start = false;
> + cfg_loop->loop_len = 0;
> + cfg_loop->loop_off = 0;

seems quite similar to last one..? Maybe a helper for common code

> + } else {
> + cfg_loop->loop_start = true;
> + cfg_loop->loop_cnt = entry->loop_cnt - 1;
> + cfg_loop->loop_len = *total_len;
> + cfg_loop->loop_off = cfg->sram_offset;
> + }
> +
> + /* Reset link and prev_off */
> +
> + cfg->addr = 0x00;
> + cfg->link = 0;
> + cfg->prev_off = 0;
> + cfg->prev_addr = cfg->addr;
> +
> + return ret;
> +}
> +
> +static int _dcc_ll_cfg_write(struct dcc_drvdata *drvdata,
> +struct dcc_config_entry *entry, struct dcc_cfg_attr *cfg, u32 *total_len)
> +{
> + u32 off;
> + int ret = 0;
> +
> + if (cfg->link) {
> + /* write new offset = 1 to continue
> + * processing the list
> + */
> + ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> +
> + if (ret)
> + return ret;
> +
> + cfg->sram_offset += 4;
> + /* Reset link and prev_off */
> + cfg->addr = 0x00;
> + cfg->prev_off = 0;
> + cfg->prev_addr = cfg->addr;
> + }
> +
> + off = entry->offset/4;
> + /* write new offset-length pair to correct position */
> + cfg->link |= ((off & BM(0, 7)) | BIT(15) | ((entry->len << 8) & BM(8, 14)));
> + cfg->link |= DCC_LINK_DESCRIPTOR;
> +
> + /* Address type */
> + cfg->addr = (entry->base >> 4) & BM(0, 27);
> + if (entry->apb_bus)
> + cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_WRITE_IND | DCC_APB_IND;
> + else
> + cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_WRITE_IND | DCC_AHB_IND;
> + ret = dcc_sram_writel(drvdata, cfg->addr, cfg->sram_offset);
> +
> + if (ret)
> + return ret;
> + cfg->sram_offset += 4;
> +
> + ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> + if (ret)
> + return ret;
> + cfg->sram_offset += 4;
> +
> + ret = dcc_sram_writel(drvdata, entry->write_val, cfg->sram_offset);
> +
> + if (ret)
> + return ret;
> +
> + cfg->sram_offset += 4;
> + cfg->addr = 0x00;
> + cfg->link = 0;
> + return ret;
> +}
> +
> +static int _dcc_ll_cfg_default(struct dcc_drvdata *drvdata,
> +struct dcc_config_entry *entry, struct dcc_cfg_attr *cfg, u32 *pos, u32 *total_len)
> +{
> + int ret = 0;
> + u32 off;
> +
> + cfg->addr = (entry->base >> 4) & BM(0, 27);
> +
> + if (entry->apb_bus)
> + cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_READ_IND | DCC_APB_IND;
> + else
> + cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_READ_IND | DCC_AHB_IND;
> +
> + off = entry->offset/4;
> +
> + *total_len += entry->len * 4;
> +
> + if (!cfg->prev_addr || cfg->prev_addr != cfg->addr || cfg->prev_off > off) {
> + /* Check if we need to write prev link entry */
> + if (cfg->link) {
> + ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> + if (ret)
> + return ret;
> + cfg->sram_offset += 4;
> + }
> + dev_dbg(drvdata->dev, "DCC: sram address 0x%x\n", cfg->sram_offset);
> +
> + /* Write address */
> + ret = dcc_sram_writel(drvdata, cfg->addr, cfg->sram_offset);
> +
> + if (ret)
> + return ret;
> +
> + cfg->sram_offset += 4;
> +
> + /* Reset link and prev_off */
> + cfg->link = 0;
> + cfg->prev_off = 0;
> + }
> +
> + if ((off - cfg->prev_off) > 0xFF || entry->len > MAX_DCC_LEN) {
> + dev_err(drvdata->dev, "DCC: Programming error Base: 0x%x, offset 0x%x\n",
> + entry->base, entry->offset);
> + ret = -EINVAL;
> + return ret;
> + }
> +
> + if (cfg->link) {
> + /*
> + * link already has one offset-length so new
> + * offset-length needs to be placed at
> + * bits [29:15]
> + */
> + *pos = 15;
> +
> + /* Clear bits [31:16] */
> + cfg->link &= BM(0, 14);
> + } else {
> + /*
> + * link is empty, so new offset-length needs
> + * to be placed at bits [15:0]
> + */
> + *pos = 0;
> + cfg->link = 1 << 15;
> + }
> +
> + /* write new offset-length pair to correct position */
> + cfg->link |= (((off-cfg->prev_off) & BM(0, 7)) | ((entry->len << 8) & BM(8, 14))) << *pos;
> +
> + cfg->link |= DCC_LINK_DESCRIPTOR;
> +
> + if (*pos) {
> + ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> + if (ret)
> + return ret;
> + cfg->sram_offset += 4;
> + cfg->link = 0;
> + }
> +
> + cfg->prev_off = off + entry->len - 1;
> + cfg->prev_addr = cfg->addr;
> + return ret;
> +}
> +
> +static int __dcc_ll_cfg(struct dcc_drvdata *drvdata, int curr_list)
> +{
> + int ret = 0;
> + u32 total_len, pos;
> + struct dcc_config_entry *entry;
> + struct dcc_cfg_attr cfg;
> + struct dcc_cfg_loop_attr cfg_loop;
> +
> + cfg.sram_offset = drvdata->ram_cfg * 4;
> + cfg.prev_off = 0;
> + cfg_loop.loop_off = 0;
> + total_len = 0;
> + cfg_loop.loop_len = 0;
> + cfg_loop.loop_cnt = 0;
> + cfg_loop.loop_start = false;
> + cfg.prev_addr = 0;
> + cfg.addr = 0;
> + cfg.link = 0;

again use memset for these

> +
> + list_for_each_entry(entry, &drvdata->cfg_head[curr_list], list) {
> + switch (entry->desc_type) {
> + case DCC_READ_WRITE_TYPE:
> + {

checkpatch should have told you this is not typical kernel style, pls
fix this and many other things before we process further

--
~Vinod

Next message: Lizhi Hou: "[PATCH V3 XRT Alveo 10/18] fpga: xrt: VSEC platform driver"
Previous message: Stephen Rothwell: "linux-next: build failure after merge of the gpio-brgl tree"
In reply to: kernel test robot: "Re: [PATCH V0 3/6] soc: qcom: dcc:Add driver support for Data Capture and Compare unit(DCC)"
Next in thread: schowdhu: "Re: [PATCH V0 3/6] soc: qcom: dcc:Add driver support for Data Capture and Compare unit(DCC)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]