Re: [PATCH v10 4/8] coresight: tmc: Enable panic sync handling

Hi,

Please ignore this message, was sent by mistake.
Atually a repetition of message sent earlier.

Thanks.

On 2024-10-29 at 11:54:41, Linu Cherian (lcherian@marvell.com) wrote:

Hi Suzuki,

On 2024-10-17 at 17:42:21, Linu Cherian (lcherian@marvell.com) wrote:

quoted

Hi Suzuki,

On 2024-10-01 at 22:13:12, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

quoted

Hi Linu

On 16/09/2024 11:34, Linu Cherian wrote:

quoted

- Get reserved region from device tree node for metadata
- Define metadata format for TMC
- Add TMC ETR panic sync handler that syncs register snapshot
   to metadata region
- Add TMC ETF panic sync handler that syncs register snapshot
   to metadata region and internal SRAM to reserved trace buffer
   region.

The patch looks good overall. Some minor comments below.

quoted

Signed-off-by: Linu Cherian <redacted>
---
Changelog from v9:
- Add common helper function of_tmc_get_reserved_resource_by_name
   for better code reuse
- Inorder to keep the reserved buffer validity and crashdata validity
   independent, is_tmc_reserved_region_valid renamed to tmc_has_reserved_buffer
- drvdata->crash_tbuf renamed to drvdata->resrv_buf
- New fields added to crash metadata: version, ffcr, ffsr, mode
- Defined crashdata version with Major version 1, Minor version 0
- Set version while creating crashdata record
- Removed Reviewed-by tag due to the above changes
  .../hwtracing/coresight/coresight-tmc-core.c  | 14 ++++
  .../hwtracing/coresight/coresight-tmc-etf.c   | 77 ++++++++++++++++++
  .../hwtracing/coresight/coresight-tmc-etr.c   | 78 +++++++++++++++++++
  drivers/hwtracing/coresight/coresight-tmc.h   | 66 ++++++++++++++++
  4 files changed, 235 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index 0764c21aba0f..54bf8ae2bff8 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c

@@ -445,6 +445,20 @@ static void tmc_get_reserved_region(struct device *parent)
  	drvdata->resrv_buf.paddr = res.start;
  	drvdata->resrv_buf.size  = resource_size(&res);
+
+	if (of_tmc_get_reserved_resource_by_name(parent, "metadata", &res))
+		return;
+
+	drvdata->crash_mdata.vaddr = memremap(res.start,
+					       resource_size(&res),
+					       MEMREMAP_WC);
+	if (IS_ERR_OR_NULL(drvdata->crash_mdata.vaddr)) {
+		dev_err(parent, "Metadata memory mapping failed\n");
+		return;
+	}
+
+	drvdata->crash_mdata.paddr = res.start;
+	drvdata->crash_mdata.size  = resource_size(&res);
  }
  /* Detect and initialise the capabilities of a TMC ETR */

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index d4f641cd9de6..d77ec9307e98 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c

@@ -590,6 +590,78 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
  	return to_read;
  }
+static int tmc_panic_sync_etf(struct coresight_device *csdev)
+{
+	u32 val;
+	struct csdev_access *csa;
+	struct tmc_crash_metadata *mdata;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	csa = &drvdata->csdev->access;
+	mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
+
+	/* Make sure we have valid reserved memory */
+	if (!tmc_has_reserved_buffer(drvdata) ||
+	    !tmc_has_crash_mdata_buffer(drvdata))
+		return 0;
+
+	tmc_crashdata_set_invalid(drvdata);
+
+	CS_UNLOCK(drvdata->base);
+
+	/* Proceed only if ETF is enabled or configured as sink */
+	val = readl(drvdata->base + TMC_CTL);
+	if (!(val & TMC_CTL_CAPT_EN))
+		goto out;
+

minor nit : Since the check below is "covered" by the same comment
above, please drop the extra line here to make it clear that "we check
for sink" by checking the "MODE == CIRCULAR_BUFFER".

Ack.

quoted

quoted

+	val = readl(drvdata->base + TMC_MODE);
+	if (val != TMC_MODE_CIRCULAR_BUFFER)
+		goto out;
+
+	val = readl(drvdata->base + TMC_FFSR);
+	/* Do manual flush and stop only if its not auto-stopped */
+	if (!(val & TMC_FFSR_FT_STOPPED)) {
+		dev_info(&csdev->dev,
+			 "%s: Triggering manual flush\n", __func__);

Please drop the ^^^ line. We don't want to do anything like that from a
panic callback.

Ack.

Are we okay with converting this to dev_dbg, as this could be quite
helpful to understand if the CTI trigger has occured or not.

quoted

quoted

quoted

+		tmc_flush_and_stop(drvdata);
+	} else
+		tmc_wait_for_tmcready(drvdata);
+
+	/* Sync registers from hardware to metadata region */
+	mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);

Why are we using "csa" here and not for TMC_CTL etc ? Simply drop the "csa"
and use the raw reads like above. TMC doesn't have anyother modes
of access.

Okay.

quoted

quoted

+	mdata->mode = csdev_access_relaxed_read32(csa, TMC_MODE);
+	mdata->ffcr = csdev_access_relaxed_read32(csa, TMC_FFCR);
+	mdata->ffsr = csdev_access_relaxed_read32(csa, TMC_FFSR);
+	mdata->trace_paddr = drvdata->resrv_buf.paddr;
+
+	/* Sync Internal SRAM to reserved trace buffer region */
+	drvdata->buf = drvdata->resrv_buf.vaddr;
+	tmc_etb_dump_hw(drvdata);
+	/* Store as per RSZ register convention */
+	mdata->size = drvdata->len >> 2;
+	mdata->version = CS_CRASHDATA_VERSION;
+
+	/*
+	 * Make sure all previous writes are completed,
+	 * before we mark valid
+	 */
+	dsb(sy);

I don't think this matters much, as this would only be read by a
secondary kernel. In the worst case, you only need `dmb(ish)` to make
sure the writes are visible before valid is set to true.

Ack. Will change that.

quoted

quoted

+	mdata->valid = true;
+	/*
+	 * Below order need to maintained, since crc of metadata
+	 * is dependent on first
+	 */
+	mdata->crc32_tdata = find_crash_tracedata_crc(drvdata, mdata);
+	mdata->crc32_mdata = find_crash_metadata_crc(mdata);
+
+	tmc_disable_hw(drvdata);
+
+	dev_info(&csdev->dev, "%s: success\n", __func__);

Please no "prints" from a panic call back, unless it absolutely CRITICAL.

Ack.

Are we okay with converting this to dev_dbg, as this could aid in
debugging to understand if kernel has populated valid metadata.

Thanks.
Linu Cherian.