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:
Hi Suzuki,
On 2024-10-01 at 22:13:12, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:
Hi Linu
On 16/09/2024 11:34, Linu Cherian wrote:
- 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.
Signed-off-by: Linu Cherian lcherian@marvell.com
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.
- 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.
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.
- 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.
- 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.