[PATCH V22 2/3] misc: dcc: Add driver support for Data Capture and Compare unit(DCC)
Souradeep Chowdhury
quic_schowdhu at quicinc.com
Wed Apr 19 00:00:19 PDT 2023
On 4/18/2023 9:15 PM, Greg Kroah-Hartman wrote:
> On Tue, Apr 18, 2023 at 08:52:36PM +0530, 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 user inputs via
>> the debugfs interface. The user gives addresses as inputs and these
>> addresses are stored in the dcc sram. In case of a system crash or a
>> manual software trigger by the user through the debugfs interface, the
>> dcc captures and stores the values at these addresses. This patch
>> contains the driver which has all the methods pertaining to the debugfs
>> interface, auxiliary functions to support all the four fundamental
>> operations of dcc namely read, write, read/modify/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 software triggers as well based on user inputs.
>>
>> Also add the documentation for debugfs entries which explains the
>> functionalities of each debugfs file that has been created for dcc.
>
> I see no documentation entries in this commit :(
My apologies, this patch was given from qcom-next tree which already has
the documentation merged. Will include it from the next versions.
>
>> The following is the justification of using debugfs interface over the
>> other alternatives like sysfs/ioctls
>>
>> i) As can be seen from the debugfs attribute descriptions, some of the
>> debugfs attribute files here contains multiple arguments which needs to
>> be accepted from the user. This goes against the design style of sysfs.
>>
>> ii) The user input patterns have been made simple and convenient in this
>> case with the use of debugfs interface as user doesn't need to shuffle
>> between different files to execute one instruction as was the case on
>> using other alternatives.
>
> Why do you have debugfs and also a misc device? How are they related?
> Why both? Why not just one? What userspace tools are going to use
> either of these interfaces and where are they published to show how this
> all was tested?
DCC has two fundamental steps of usage:-
1.Configuring the register addresses on the dcc_sram which is done by
user through the debugfs interface. For example:-
echo R 0x10c004 > /sys/kernel/debug/dcc/../3/config
Here we are configuring the register addresses for list 3, the 'R'
indicates a read operation, so this register value will be read
in case of a software trigger or kernel panic/watchdog bite and
dumped into the dcc_sram.
2.The dcc_sram content is exposed to the user in the form of a misc
device. The user can parse the content of this dcc_sram to get the
register values. There is an opensource parser available for dcc at
the following location:-
https://git.codelinaro.org/clo/le/platform/vendor/qcom-opensource/tools/-/tree/opensource-tools.lnx.1.0.r176-rel/dcc_parser
>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu at quicinc.com>
>> Reviewed-by: Alex Elder <elder at linaro.org>
>> ---
>> drivers/misc/Kconfig | 8 +
>> drivers/misc/Makefile | 1 +
>> drivers/misc/dcc.c | 1300 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 1309 insertions(+)
>> create mode 100644 drivers/misc/dcc.c
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 433aa41..e2bc652 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -276,6 +276,14 @@ config QCOM_COINCELL
>> to maintain PMIC register and RTC state in the absence of
>> external power.
>>
>> +config QCOM_DCC
>> + tristate "Qualcomm Technologies, Inc. Data Capture and Compare(DCC) 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.
>
>
> What is the module name?
It's qcom-dcc, will update the name here.
>
>> +
>> config QCOM_FASTRPC
>> tristate "Qualcomm FastRPC"
>> depends on ARCH_QCOM || COMPILE_TEST
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 56de439..6fa8efa 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -16,6 +16,7 @@ obj-$(CONFIG_TIFM_CORE) += tifm_core.o
>> obj-$(CONFIG_TIFM_7XX1) += tifm_7xx1.o
>> obj-$(CONFIG_PHANTOM) += phantom.o
>> obj-$(CONFIG_QCOM_COINCELL) += qcom-coincell.o
>> +obj-$(CONFIG_QCOM_DCC) += dcc.o
>
> Why such a generic name? Shouldn't it be qcom-dcc?
Ack
>
>
>
>> obj-$(CONFIG_QCOM_FASTRPC) += fastrpc.o
>> obj-$(CONFIG_SENSORS_BH1770) += bh1770glc.o
>> obj-$(CONFIG_SENSORS_APDS990X) += apds990x.o
>> diff --git a/drivers/misc/dcc.c b/drivers/misc/dcc.c
>> new file mode 100644
>> index 0000000..7231ed9
>> --- /dev/null
>> +++ b/drivers/misc/dcc.c
>> @@ -0,0 +1,1300 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>
> No work happened on this code in 2022? All 22 of these entries were
> only in 2021 and 2023?
Ack
>
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/delay.h>
>> +#include <linux/fs.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/miscdevice.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 STATUS_READY_TIMEOUT 5000 /* microseconds */
>> +
>> +/* DCC registers */
>> +#define DCC_HW_INFO 0x04
>> +#define DCC_LL_NUM_INFO 0x10
>> +#define DCC_STATUS(vers) ((vers) == 1 ? 0x0c : 0x1c)
>
> Why isn't this just an inline function?
Ack. Will make this inline
>
>> +#define DCC_LL_LOCK 0x00
>> +#define DCC_LL_CFG 0x04
>> +#define DCC_LL_BASE 0x08
>> +#define DCC_FD_BASE 0x0c
>> +#define DCC_LL_TIMEOUT 0x10
>> +#define DCC_LL_INT_ENABLE 0x18
>> +#define DCC_LL_INT_STATUS 0x1c
>> +#define DCC_LL_SW_TRIGGER 0x2c
>> +#define DCC_LL_BUS_ACCESS_STATUS 0x30
>> +
>> +/* Default value used if a bit 6 in the HW_INFO register is set. */
>> +#define DCC_FIX_LOOP_OFFSET 16
>> +
>> +/* Mask to find version info from HW_Info register */
>> +#define DCC_VER_INFO_MASK BIT(9)
>> +
>> +#define MAX_DCC_OFFSET GENMASK(9, 2)
>> +#define MAX_DCC_LEN GENMASK(6, 0)
>> +#define MAX_LOOP_CNT GENMASK(7, 0)
>> +#define MAX_LOOP_ADDR 10
>> +
>> +#define DCC_ADDR_DESCRIPTOR 0x00
>> +#define DCC_ADDR_LIMIT 27
>> +#define DCC_WORD_SIZE sizeof(u32)
>> +#define DCC_ADDR_RANGE_MASK GENMASK(31, 4)
>> +#define DCC_LOOP_DESCRIPTOR BIT(30)
>> +#define DCC_RD_MOD_WR_DESCRIPTOR BIT(31)
>> +#define DCC_LINK_DESCRIPTOR GENMASK(31, 30)
>> +#define DCC_STATUS_MASK GENMASK(1, 0)
>> +#define DCC_LOCK_MASK BIT(0)
>> +#define DCC_LOOP_OFFSET_MASK BIT(6)
>> +#define DCC_TRIGGER_MASK BIT(9)
>> +
>> +#define DCC_WRITE_MASK BIT(15)
>> +#define DCC_WRITE_OFF_MASK GENMASK(7, 0)
>> +#define DCC_WRITE_LEN_MASK GENMASK(14, 8)
>> +
>> +#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_VER_MASK2 GENMASK(5, 0)
>> +
>> +#define DCC_SRAM_WORD_LENGTH 4
>> +
>> +#define DCC_RD_MOD_WR_ADDR 0xC105E
>> +
>> +enum dcc_descriptor_type {
>> + DCC_READ_TYPE,
>> + DCC_LOOP_TYPE,
>> + DCC_READ_WRITE_TYPE,
>> + DCC_WRITE_TYPE
>> +};
>> +
>> +struct dcc_config_entry {
>> + u32 base;
>> + u32 offset;
>> + u32 len;
>> + u32 loop_cnt;
>> + u32 write_val;
>> + u32 mask;
>> + bool apb_bus;
>> + enum dcc_descriptor_type desc_type;
>> + struct list_head list;
>> +};
>
> No documentation for this structure?
Ack. Will add documentation to this structure as well.
>
>> +
>> +/**
>> + * struct dcc_drvdata - configuration information related to a dcc device
>> + * @base: Base Address of the dcc device
>> + * @dev: The device attached to the driver data
>> + * @mutex: Lock to protect access and manipulation of dcc_drvdata
>> + * @ram_base: Base address for the SRAM dedicated for the dcc device
>> + * @ram_size: Total size of the SRAM dedicated for the dcc device
>> + * @ram_offset: Offset to the SRAM dedicated for dcc device
>> + * @mem_map_ver: Memory map version of DCC hardware
>> + * @ram_cfg: Used for address limit calculation for dcc
>> + * @ram_start: Starting address of DCC SRAM
>> + * @sram_dev: Miscellaneous device equivalent of dcc SRAM
>> + * @cfg_head: Points to the head of the linked list of addresses
>> + * @dbg_dir: The dcc debugfs directory under which all the debugfs files are placed
>> + * @nr_link_list: Total number of linkedlists supported by the DCC configuration
>> + * @loop_shift: Loop offset bits range for the addresses
>> + * @enable_bitmap: Bitmap to capture the enabled status of each linked list of addresses
>> + */
>> +struct dcc_drvdata {
>> + void __iomem *base;
>> + void __iomem *ram_base;
>> + struct device *dev;
>
> Why do you need this back-pointer here?
This is getting used at multiple places to log
dev_err and also for resource allocation using
devm_kzalloc.
>
>> + struct mutex mutex;
>> + size_t ram_size;
>> + size_t ram_offset;
>> + int mem_map_ver;
>> + unsigned int ram_cfg;
>> + unsigned int ram_start;
>> + struct miscdevice sram_dev;
>
> Wait, this is the struct device, right? Or not?
miscdevice here represents the dcc_sram, an io-memory
dedicated to dcc for configuring/storing register values.
Whereas struct device represents the dcc_device which can
be used to write control signals on the bus to handle dcc
hardware operation sequence(like config_reset,sw_trigger etc.)
>
>> + struct list_head *cfg_head;
>> + struct dentry *dbg_dir;
>
> Why is this needed and not just looked up when necessary?
This needs to be passed while creating sub-directories and also
while removing. Rather than looking up everytime,saving and
re-using this in here.
>
>> + size_t nr_link_list;
>> + u8 loop_shift;
>> + unsigned long *enable_bitmap;
>
> So this is a list of bitmaps? Why "unsigned long"? Why not u64?
Ack
>
>> +};
>> +
>> +struct dcc_cfg_attr {
>> + u32 addr;
>> + u32 prev_addr;
>> + u32 prev_off;
>> + u32 link;
>> + u32 sram_offset;
>> +};
>> +
>> +struct dcc_cfg_loop_attr {
>> + u32 loop_cnt;
>> + u32 loop_len;
>> + u32 loop_off;
>> + bool loop_start;
>> +};
>> +
>> +static inline u32 dcc_list_offset(int version)
>> +{
>> + return version == 1 ? 0x1c : version == 2 ? 0x2c : 0x34;
>> +}
>
> Ah, you do have an inline function for the above mentioned macro.
> Please drop the macro.
>
> And write this inline function out to be readable, single-level ?:
> comments are impossible to read, let alone double-level ones.
>
> Write code for people first, compilers second. You gain nothing by
> making this terse except to confuse people.
Ack. This inline function is different from the previous macro.
Will keep both as inline functions.
>
>> +
>> +static inline void dcc_list_writel(struct dcc_drvdata *drvdata,
>> + u32 ll, u32 val, u32 off)
>> +{
>> + u32 offset = dcc_list_offset(drvdata->mem_map_ver) + off;
>> +
>> + writel(val, drvdata->base + ll * 0x80 + offset);
>
> What is this magic 0x80 for?
This is the list offset needed for address calculation as per the
dcc-hardware specification. Will declare a macro for this.
>
>> +}
>> +
>> +static inline u32 dcc_list_readl(struct dcc_drvdata *drvdata, u32 ll, u32 off)
>> +{
>> + u32 offset = dcc_list_offset(drvdata->mem_map_ver) + off;
>> +
>> + return readl(drvdata->base + ll * 0x80 + offset);
>
> Again, where did 0x80 come from?
Same as above.
>
>> +}
>> +
>> +static void dcc_sram_write_auto(struct dcc_drvdata *drvdata,
>> + u32 val, u32 *off)
>> +{
>> + /* If the overflow condition is met increment the offset
>> + * and return to indicate that overflow has occurred
>> + */
>> + if (unlikely(*off > drvdata->ram_size - 4)) {
>> + *off += 4;
>> + return;
>
> You didn't indicate anything here, all you did was succeed at the call,
> the caller has no way of ever knowing this failed.
>
> Why not return an error?
As per previous discussions it was decided to perform the write
speculatively. So that while writing to the dcc_sram if we exceed
the memory size, dcc will skip the write and keep incrementing
the offset. In the method "dcc_emit_config" we have the check to
finally detect if we have exceeded the sram offset
if (cfg.sram_offset + total_len > drvdata->ram_size) {
cfg.sram_offset += total_len;
goto overstep;
}
>
>> + }
>> +
>> + writel(val, drvdata->ram_base + *off);
>> +
>> + *off += 4;
>
> See, same modification as your "error" above.
>
> How was this tested?
This increment is needed to update the offset for the next memory
position in dcc_sram.
>
>> +static int dcc_emit_config(struct dcc_drvdata *drvdata, unsigned int curr_list)
>> +{
>> + int ret;
>> + u32 total_len, pos;
>> + struct dcc_config_entry *entry;
>> + struct dcc_cfg_attr cfg;
>> + struct dcc_cfg_loop_attr cfg_loop;
>> +
>> + memset(&cfg, 0, sizeof(cfg));
>> + memset(&cfg_loop, 0, sizeof(cfg_loop));
>
> Why are these large structures on the stack?
cfg_loop is needed for offset calculation in case of dcc loop
instructions based on the way it needs to be configured in dcc_sram
for dcc hardware to interpret it. entry, cfg is a generic structure used
across all dcc instructions. All these structures are needed for the
memory checks after we are done with configuring all the dcc instructions.
>
> And if on the stack, why not have the compiler initialize them to 0 for
> you automatically?
Ack
>
> I stopped reviewing here...
>
> greg k-h
More information about the linux-arm-kernel
mailing list