From: Bjorn Andersson <andersson@kernel.org>
To: Praveen Talari <praveen.talari@oss.qualcomm.com>
Cc: Konrad Dybcio <konradybcio@kernel.org>,
Steven Rostedt <rostedt@goodmis.org>,
Masami Hiramatsu <mhiramat@kernel.org>,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
Mark Brown <broonie@kernel.org>,
linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-trace-kernel@vger.kernel.org, linux-spi@vger.kernel.org
Subject: Re: [PATCH v2 1/2] soc: qcom: geni-se: trace: Add trace event support for GENI SE registers dump
Date: Sat, 11 Jul 2026 10:54:16 -0500 [thread overview]
Message-ID: <alJmNx7Zhkh_qpeX@baldur> (raw)
In-Reply-To: <20260711-add-tracepoints-for-se-reg-dump-v2-1-ca1e9ba62359@oss.qualcomm.com>
On Sat, Jul 11, 2026 at 12:18:41AM +0530, Praveen Talari wrote:
> Add a new trace event header for the Qualcomm GENI Serial Engine (SE)
> framework providing a geni_se_regs tracepoint. This tracepoint
> captures a comprehensive snapshot of the GENI SE hardware state in a
> single trace record, making it possible to correlate register values at
> a precise point in time without multiple sequential reads.
As stated in [a], start your commit message by describing the problem
you're trying to solve, the reader shouldn't have to read the whole
commit message to get an idea about why the change is needed.
[a] https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
>
> The trace event records the following register groups:
>
> - Main/secondary command and IRQ status (M_CMD0, S_CMD0, M/S_IRQ_STATUS)
> - Engine status, IOS, and command control/error registers
> - TX/RX FIFO status and watermark registers (including RFR watermark)
> - M/S GP length registers
> - DMA TX/RX IRQ, enable, length, pointer, attribute, and burst registers
> - DMA interface enable, general config, QSB trans config, and debug
> - M/S IRQ enable, GSI event enable, and top-level SE IRQ enable
> - Serial master/slave clock config, general config, output control,
> clock control RO, FIFO interface disable, and FW multilock MSA
> - Clock select register
>
> Having all these registers captured atomically in a single ftrace record
> allows drivers built on top of the GENI SE framework (serial, SPI, I2C)
> to invoke this tracepoint on error paths and reconstruct the full engine
> state during post-mortem analysis without instrumenting each driver
> separately.
I'm not rejecting the patch, but isn't this exactly why we have e.g.
DCC? To allow grabbing a set of registers in adverse events - without
having to write debug code like this in each driver?
Regards,
Bjorn
>
> Acked-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
> include/linux/soc/qcom/geni-se.h | 38 +++++++++
> include/trace/events/qcom_geni_se.h | 157 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 195 insertions(+)
>
> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index c5e6ab85df09..8c08c1917374 100644
> --- a/include/linux/soc/qcom/geni-se.h
> +++ b/include/linux/soc/qcom/geni-se.h
> @@ -81,13 +81,16 @@ struct geni_se {
> };
>
> /* Common SE registers */
> +#define GENI_GENERAL_CFG 0x10
> #define GENI_FORCE_DEFAULT_REG 0x20
> #define GENI_OUTPUT_CTRL 0x24
> #define SE_GENI_STATUS 0x40
> #define GENI_SER_M_CLK_CFG 0x48
> #define GENI_SER_S_CLK_CFG 0x4c
> +#define GENI_CLK_CTRL_RO 0x60
> #define GENI_IF_DISABLE_RO 0x64
> #define GENI_FW_REVISION_RO 0x68
> +#define GENI_FW_MULTILOCK_MSA_RO 0x74
> #define SE_GENI_CLK_SEL 0x7c
> #define SE_GENI_CFG_SEQ_START 0x84
> #define SE_GENI_DMA_MODE_EN 0x258
> @@ -98,6 +101,8 @@ struct geni_se {
> #define SE_GENI_M_IRQ_CLEAR 0x618
> #define SE_GENI_M_IRQ_EN_SET 0x61c
> #define SE_GENI_M_IRQ_EN_CLEAR 0x620
> +#define M_CMD_ERR_STATUS 0x624
> +#define M_FW_ERR_STATUS 0x628
> #define SE_GENI_S_CMD0 0x630
> #define SE_GENI_S_CMD_CTRL_REG 0x634
> #define SE_GENI_S_IRQ_STATUS 0x640
> @@ -115,15 +120,42 @@ struct geni_se {
> #define SE_GENI_IOS 0x908
> #define SE_GENI_M_GP_LENGTH 0x910
> #define SE_GENI_S_GP_LENGTH 0x914
> +/* TX DMA registers */
> +#define SE_DMA_TX_PTR_L 0xc30
> +#define SE_DMA_TX_PTR_H 0xc34
> +#define SE_DMA_TX_ATTR 0xc38
> +#define SE_DMA_TX_LEN 0xc3c
> #define SE_DMA_TX_IRQ_STAT 0xc40
> #define SE_DMA_TX_IRQ_CLR 0xc44
> +#define SE_DMA_TX_IRQ_EN 0xc48
> +#define SE_DMA_TX_IRQ_EN_SET 0xc4c
> +#define SE_DMA_TX_IRQ_EN_CLR 0xc50
> +#define SE_DMA_TX_LEN_IN 0xc54
> #define SE_DMA_TX_FSM_RST 0xc58
> +#define SE_DMA_TX_MAX_BURST 0xc5c
> +/* RX DMA registers */
> +#define SE_DMA_RX_PTR_L 0xd30
> +#define SE_DMA_RX_PTR_H 0xd34
> +#define SE_DMA_RX_ATTR 0xd38
> +#define SE_DMA_RX_LEN 0xd3c
> #define SE_DMA_RX_IRQ_STAT 0xd40
> #define SE_DMA_RX_IRQ_CLR 0xd44
> +#define SE_DMA_RX_IRQ_EN 0xd48
> +#define SE_DMA_RX_IRQ_EN_SET 0xd4c
> +#define SE_DMA_RX_IRQ_EN_CLR 0xd50
> #define SE_DMA_RX_LEN_IN 0xd54
> #define SE_DMA_RX_FSM_RST 0xd58
> +#define SE_DMA_RX_MAX_BURST 0xd5c
> +/* DMA general / debug registers */
> +#define SE_GSI_EVENT_EN 0xe18
> +#define SE_IRQ_EN 0xe1c
> +#define DMA_IF_EN_RO 0xe20
> #define SE_HW_PARAM_0 0xe24
> #define SE_HW_PARAM_1 0xe28
> +#define DMA_GENERAL_CFG 0xe30
> +#define SE_DMA_QSB_TRANS_CFG 0xe38
> +#define SE_DMA_DEBUG_REG0 0xe40
> +#define SE_DMA_IF_EN 0x2004
>
> /* GENI_FORCE_DEFAULT_REG fields */
> #define FORCE_DEFAULT BIT(0)
> @@ -269,6 +301,12 @@ struct geni_se {
> #define RX_GENI_GP_IRQ_EXT GENMASK(13, 12)
> #define RX_GENI_CANCEL_IRQ BIT(14)
>
> +/* SE_DMA_DEBUG_REG0 fields */
> +#define DMA_TX_ACTIVE BIT(0)
> +#define DMA_RX_ACTIVE BIT(1)
> +#define DMA_TX_STATE GENMASK(7, 4)
> +#define DMA_RX_STATE GENMASK(11, 8)
> +
> /* SE_HW_PARAM_0 fields */
> #define TX_FIFO_WIDTH_MSK GENMASK(29, 24)
> #define TX_FIFO_WIDTH_SHFT 24
> diff --git a/include/trace/events/qcom_geni_se.h b/include/trace/events/qcom_geni_se.h
> new file mode 100644
> index 000000000000..4a6e1ba2d147
> --- /dev/null
> +++ b/include/trace/events/qcom_geni_se.h
> @@ -0,0 +1,157 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM qcom_geni_se
> +
> +#if !defined(_TRACE_QCOM_GENI_SE_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_QCOM_GENI_SE_H
> +
> +#include <linux/io.h>
> +#include <linux/tracepoint.h>
> +#include <linux/soc/qcom/geni-se.h>
> +
> +TRACE_EVENT(geni_se_regs,
> + TP_PROTO(struct geni_se *se),
> +
> + TP_ARGS(se),
> +
> + TP_STRUCT__entry(__string(geni_se_name, dev_name(se->dev))
> + __field(u32, geni_se_m_cmd0)
> + __field(u32, geni_se_m_irq_status)
> + __field(u32, geni_se_s_cmd0)
> + __field(u32, geni_se_s_irq_status)
> + __field(u32, geni_se_status)
> + __field(u32, geni_se_ios)
> + __field(u32, geni_se_m_cmd_ctrl)
> + __field(u32, geni_se_m_cmd_err)
> + __field(u32, geni_se_m_fw_err)
> + __field(u32, geni_se_tx_fifo_status)
> + __field(u32, geni_se_rx_fifo_status)
> + __field(u32, geni_se_tx_watermark)
> + __field(u32, geni_se_rx_watermark)
> + __field(u32, geni_se_rx_watermark_rfr)
> + __field(u32, geni_se_m_gp_length)
> + __field(u32, geni_se_s_gp_length)
> + __field(u32, geni_se_dma_tx_irq)
> + __field(u32, geni_se_dma_rx_irq)
> + __field(u32, geni_se_dma_tx_irq_en)
> + __field(u32, geni_se_dma_rx_irq_en)
> + __field(u32, geni_se_dma_rx_len)
> + __field(u32, geni_se_dma_rx_len_in)
> + __field(u32, geni_se_dma_tx_len)
> + __field(u32, geni_se_dma_tx_len_in)
> + __field(u32, geni_se_dma_tx_ptr_l)
> + __field(u32, geni_se_dma_tx_ptr_h)
> + __field(u32, geni_se_dma_rx_ptr_l)
> + __field(u32, geni_se_dma_rx_ptr_h)
> + __field(u32, geni_se_dma_tx_attr)
> + __field(u32, geni_se_dma_tx_max_burst)
> + __field(u32, geni_se_dma_rx_attr)
> + __field(u32, geni_se_dma_rx_max_burst)
> + __field(u32, geni_se_dma_if_en)
> + __field(u32, geni_se_dma_if_en_ro)
> + __field(u32, geni_se_dma_general_cfg)
> + __field(u32, geni_se_dma_qsb_trans_cfg)
> + __field(u32, geni_se_dma_dbg)
> + __field(u32, geni_se_m_irq_en)
> + __field(u32, geni_se_s_irq_en)
> + __field(u32, geni_se_gsi_event_en)
> + __field(u32, geni_se_irq_en)
> + __field(u32, geni_se_ser_m_clk_cfg)
> + __field(u32, geni_se_ser_s_clk_cfg)
> + __field(u32, geni_se_general_cfg)
> + __field(u32, geni_se_output_ctrl)
> + __field(u32, geni_se_clk_ctrl_ro)
> + __field(u32, geni_se_fifo_if_disable)
> + __field(u32, geni_se_fw_multilock_msa)
> + __field(u32, geni_se_clk_sel)
> + ),
> +
> + TP_fast_assign(__assign_str(geni_se_name);
> + __entry->geni_se_m_cmd0 = readl(se->base + SE_GENI_M_CMD0);
> + __entry->geni_se_m_irq_status = readl(se->base + SE_GENI_M_IRQ_STATUS);
> + __entry->geni_se_s_cmd0 = readl(se->base + SE_GENI_S_CMD0);
> + __entry->geni_se_s_irq_status = readl(se->base + SE_GENI_S_IRQ_STATUS);
> + __entry->geni_se_status = readl(se->base + SE_GENI_STATUS);
> + __entry->geni_se_ios = readl(se->base + SE_GENI_IOS);
> + __entry->geni_se_m_cmd_ctrl = readl(se->base + SE_GENI_M_CMD_CTRL_REG);
> + __entry->geni_se_m_cmd_err = readl(se->base + M_CMD_ERR_STATUS);
> + __entry->geni_se_m_fw_err = readl(se->base + M_FW_ERR_STATUS);
> + __entry->geni_se_tx_fifo_status = readl(se->base + SE_GENI_TX_FIFO_STATUS);
> + __entry->geni_se_rx_fifo_status = readl(se->base + SE_GENI_RX_FIFO_STATUS);
> + __entry->geni_se_tx_watermark = readl(se->base + SE_GENI_TX_WATERMARK_REG);
> + __entry->geni_se_rx_watermark = readl(se->base + SE_GENI_RX_WATERMARK_REG);
> + __entry->geni_se_rx_watermark_rfr = readl(se->base + SE_GENI_RX_RFR_WATERMARK_REG);
> + __entry->geni_se_m_gp_length = readl(se->base + SE_GENI_M_GP_LENGTH);
> + __entry->geni_se_s_gp_length = readl(se->base + SE_GENI_S_GP_LENGTH);
> + __entry->geni_se_dma_tx_irq = readl(se->base + SE_DMA_TX_IRQ_STAT);
> + __entry->geni_se_dma_rx_irq = readl(se->base + SE_DMA_RX_IRQ_STAT);
> + __entry->geni_se_dma_tx_irq_en = readl(se->base + SE_DMA_TX_IRQ_EN);
> + __entry->geni_se_dma_rx_irq_en = readl(se->base + SE_DMA_RX_IRQ_EN);
> + __entry->geni_se_dma_rx_len = readl(se->base + SE_DMA_RX_LEN);
> + __entry->geni_se_dma_rx_len_in = readl(se->base + SE_DMA_RX_LEN_IN);
> + __entry->geni_se_dma_tx_len = readl(se->base + SE_DMA_TX_LEN);
> + __entry->geni_se_dma_tx_len_in = readl(se->base + SE_DMA_TX_LEN_IN);
> + __entry->geni_se_dma_tx_ptr_l = readl(se->base + SE_DMA_TX_PTR_L);
> + __entry->geni_se_dma_tx_ptr_h = readl(se->base + SE_DMA_TX_PTR_H);
> + __entry->geni_se_dma_rx_ptr_l = readl(se->base + SE_DMA_RX_PTR_L);
> + __entry->geni_se_dma_rx_ptr_h = readl(se->base + SE_DMA_RX_PTR_H);
> + __entry->geni_se_dma_tx_attr = readl(se->base + SE_DMA_TX_ATTR);
> + __entry->geni_se_dma_tx_max_burst = readl(se->base + SE_DMA_TX_MAX_BURST);
> + __entry->geni_se_dma_rx_attr = readl(se->base + SE_DMA_RX_ATTR);
> + __entry->geni_se_dma_rx_max_burst = readl(se->base + SE_DMA_RX_MAX_BURST);
> + __entry->geni_se_dma_if_en = readl(se->base + SE_DMA_IF_EN);
> + __entry->geni_se_dma_if_en_ro = readl(se->base + DMA_IF_EN_RO);
> + __entry->geni_se_dma_general_cfg = readl(se->base + DMA_GENERAL_CFG);
> + __entry->geni_se_dma_qsb_trans_cfg = readl(se->base + SE_DMA_QSB_TRANS_CFG);
> + __entry->geni_se_dma_dbg = readl(se->base + SE_DMA_DEBUG_REG0);
> + __entry->geni_se_m_irq_en = readl(se->base + SE_GENI_M_IRQ_EN);
> + __entry->geni_se_s_irq_en = readl(se->base + SE_GENI_S_IRQ_EN);
> + __entry->geni_se_gsi_event_en = readl(se->base + SE_GSI_EVENT_EN);
> + __entry->geni_se_irq_en = readl(se->base + SE_IRQ_EN);
> + __entry->geni_se_ser_m_clk_cfg = readl(se->base + GENI_SER_M_CLK_CFG);
> + __entry->geni_se_ser_s_clk_cfg = readl(se->base + GENI_SER_S_CLK_CFG);
> + __entry->geni_se_general_cfg = readl(se->base + GENI_GENERAL_CFG);
> + __entry->geni_se_output_ctrl = readl(se->base + GENI_OUTPUT_CTRL);
> + __entry->geni_se_clk_ctrl_ro = readl(se->base + GENI_CLK_CTRL_RO);
> + __entry->geni_se_fifo_if_disable = readl(se->base + GENI_IF_DISABLE_RO);
> + __entry->geni_se_fw_multilock_msa = readl(se->base + GENI_FW_MULTILOCK_MSA_RO);
> + __entry->geni_se_clk_sel = readl(se->base + SE_GENI_CLK_SEL);
> + ),
> +
> + TP_printk("%s: m_cmd0=0x%08x m_irq_status=0x%08x s_cmd0=0x%08x s_irq_status=0x%08x geni_status=0x%08x geni_ios=0x%08x m_cmd_ctrl=0x%08x m_cmd_err=0x%08x m_fw_err=0x%08x tx_fifo_sts=0x%08x rx_fifo_sts=0x%08x tx_watermark=0x%08x rx_watermark=0x%08x rx_watermark_rfr=0x%08x m_gp_length=0x%08x s_gp_length=0x%08x dma_tx_irq=0x%08x dma_rx_irq=0x%08x dma_tx_irq_en=0x%08x dma_rx_irq_en=0x%08x dma_rx_len=0x%08x dma_rx_len_in=0x%08x dma_tx_len=0x%08x dma_tx_len_in=0x%08x dma_tx_ptr_l=0x%08x dma_tx_ptr_h=0x%08x dma_rx_ptr_l=0x%08x dma_rx_ptr_h=0x%08x dma_tx_attr=0x%08x dma_tx_max_burst=0x%08x dma_rx_attr=0x%08x dma_rx_max_burst=0x%08x dma_if_en=0x%08x dma_if_en_ro=0x%08x dma_general_cfg=0x%08x dma_qsb_trans_cfg=0x%08x dma_dbg=0x%08x m_irq_en=0x%08x s_irq_en=0x%08x gsi_event_en=0x%08x se_irq_en=0x%08x ser_m_clk_cfg=0x%08x ser_s_clk_cfg=0x%08x general_cfg=0x%08x output_ctrl=0x%08x clk_ctrl_ro=0x%08x fifo_if_dis=0x%08x fw_multilock_msa=0x%08x clk_sel=0x%08x",
> + __get_str(geni_se_name),
> + __entry->geni_se_m_cmd0, __entry->geni_se_m_irq_status,
> + __entry->geni_se_s_cmd0, __entry->geni_se_s_irq_status,
> + __entry->geni_se_status, __entry->geni_se_ios,
> + __entry->geni_se_m_cmd_ctrl,
> + __entry->geni_se_m_cmd_err, __entry->geni_se_m_fw_err,
> + __entry->geni_se_tx_fifo_status, __entry->geni_se_rx_fifo_status,
> + __entry->geni_se_tx_watermark, __entry->geni_se_rx_watermark,
> + __entry->geni_se_rx_watermark_rfr,
> + __entry->geni_se_m_gp_length, __entry->geni_se_s_gp_length,
> + __entry->geni_se_dma_tx_irq, __entry->geni_se_dma_rx_irq,
> + __entry->geni_se_dma_tx_irq_en, __entry->geni_se_dma_rx_irq_en,
> + __entry->geni_se_dma_rx_len, __entry->geni_se_dma_rx_len_in,
> + __entry->geni_se_dma_tx_len, __entry->geni_se_dma_tx_len_in,
> + __entry->geni_se_dma_tx_ptr_l, __entry->geni_se_dma_tx_ptr_h,
> + __entry->geni_se_dma_rx_ptr_l, __entry->geni_se_dma_rx_ptr_h,
> + __entry->geni_se_dma_tx_attr, __entry->geni_se_dma_tx_max_burst,
> + __entry->geni_se_dma_rx_attr, __entry->geni_se_dma_rx_max_burst,
> + __entry->geni_se_dma_if_en, __entry->geni_se_dma_if_en_ro,
> + __entry->geni_se_dma_general_cfg, __entry->geni_se_dma_qsb_trans_cfg,
> + __entry->geni_se_dma_dbg,
> + __entry->geni_se_m_irq_en, __entry->geni_se_s_irq_en,
> + __entry->geni_se_gsi_event_en, __entry->geni_se_irq_en,
> + __entry->geni_se_ser_m_clk_cfg, __entry->geni_se_ser_s_clk_cfg,
> + __entry->geni_se_general_cfg, __entry->geni_se_output_ctrl,
> + __entry->geni_se_clk_ctrl_ro, __entry->geni_se_fifo_if_disable,
> + __entry->geni_se_fw_multilock_msa, __entry->geni_se_clk_sel)
> +);
> +
> +#endif /* _TRACE_QCOM_GENI_SE_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
>
> --
> 2.34.1
>
next prev parent reply other threads:[~2026-07-11 15:54 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-10 18:48 [PATCH v2 0/2] Add trace event support for GENI SE registers dump Praveen Talari
2026-07-10 18:48 ` [PATCH v2 1/2] soc: qcom: geni-se: trace: " Praveen Talari
2026-07-11 15:54 ` Bjorn Andersson [this message]
2026-07-10 18:48 ` [PATCH v2 2/2] spi: qcom-geni: add GENI SE registers trace event on error paths Praveen Talari
2026-07-10 21:26 ` Mark Brown
2026-07-11 2:54 ` Praveen Talari
[not found] ` <4bcaa245-2127-45aa-bf05-48e275c0129c@oss.qualcomm.com>
2026-07-13 10:46 ` Mark Brown
2026-07-14 5:21 ` Praveen Talari
2026-07-14 10:24 ` Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alJmNx7Zhkh_qpeX@baldur \
--to=andersson@kernel.org \
--cc=broonie@kernel.org \
--cc=konradybcio@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mhiramat@kernel.org \
--cc=praveen.talari@oss.qualcomm.com \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox