Message-ID: <20240620041054.GC125816@hyd1403.caveonetworks.com>
Date: Thu, 20 Jun 2024 09:40:54 +0530
From: Linu Cherian <lcherian@...vell.com>
To: Suzuki K Poulose <suzuki.poulose@....com>
CC: <mike.leach@...aro.org>, <james.clark@....com>,
        <linux-arm-kernel@...ts.infradead.org>, <coresight@...ts.linaro.org>,
        <linux-kernel@...r.kernel.org>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <devicetree@...r.kernel.org>, <sgoutham@...vell.com>,
        <gcherian@...vell.com>
Subject: Re: [PATCH v9 4/7] coresight: tmc: Enable panic sync handling

On 2024-06-10 at 18:32:33, Suzuki K Poulose (suzuki.poulose@....com) wrote:
> On 05/06/2024 09:17, 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.
> > 
> > Signed-off-by: Linu Cherian <lcherian@...vell.com>
> > Reviewed-by: James Clark <james.clark@....com>
> > ---
> > Changelog from v8:
> > Added Reviewed-by tag.
> > 
> >   .../hwtracing/coresight/coresight-tmc-core.c  | 25 +++++++
> >   .../hwtracing/coresight/coresight-tmc-etf.c   | 72 +++++++++++++++++++
> >   .../hwtracing/coresight/coresight-tmc-etr.c   | 70 ++++++++++++++++++
> >   drivers/hwtracing/coresight/coresight-tmc.h   | 45 +++++++++++-
> >   4 files changed, 211 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> > index 6beb69d74d0a..daad08bc693d 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> > @@ -443,6 +443,31 @@ static void tmc_get_reserved_region(struct device *parent)
> >   	drvdata->crash_tbuf.paddr = res.start;
> >   	drvdata->crash_tbuf.size  = resource_size(&res);
> > +
> > +	/* Metadata region */
> > +	node = tmc_get_region_byname(parent->of_node, "metadata");
> > +	if (IS_ERR_OR_NULL(node)) {
> > +		dev_dbg(parent, "No metadata memory-region specified\n");
> > +		return;
> > +	}
> > +
> > +	rc = of_address_to_resource(node, 0, &res);
> > +	of_node_put(node);
> > +	if (rc || res.start == 0 || resource_size(&res) == 0) {
> > +		dev_err(parent, "Metadata memory is invalid\n");
> > +		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..f9569585e9f8 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > @@ -590,6 +590,73 @@ 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;
> > +
> > +	/* Make sure we have valid reserved memory */
> > +	if (!is_tmc_reserved_region_valid(csdev->dev.parent))
> > +		return 0;
> > +
> > +	mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
> > +	mdata->valid = false;
> > +
> > +	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;
> > +
> > +	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__);
> > +		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);
> > +	mdata->trc_paddr = drvdata->crash_tbuf.paddr;
> > +
> > +	/* Sync Internal SRAM to reserved trace buffer region */
> > +	drvdata->buf = drvdata->crash_tbuf.vaddr;
> > +	tmc_etb_dump_hw(drvdata);
> > +	/* Store as per RSZ register convention */
> > +	mdata->size = drvdata->len >> 2;
> > +
> > +	/*
> > +	 * Make sure all previous writes are completed,
> > +	 * before we mark valid
> > +	 */
> > +	dsb(sy);
> > +	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__);
> > +out:
> > +	CS_UNLOCK(drvdata->base);
> > +	return 0;
> > +}
> > +
> >   static const struct coresight_ops_sink tmc_etf_sink_ops = {
> >   	.enable		= tmc_enable_etf_sink,
> >   	.disable	= tmc_disable_etf_sink,
> > @@ -603,6 +670,10 @@ static const struct coresight_ops_link tmc_etf_link_ops = {
> >   	.disable	= tmc_disable_etf_link,
> >   };
> > +static const struct coresight_ops_panic tmc_etf_sync_ops = {
> > +	.sync		= tmc_panic_sync_etf,
> > +};
> > +
> >   const struct coresight_ops tmc_etb_cs_ops = {
> >   	.sink_ops	= &tmc_etf_sink_ops,
> >   };
> > @@ -610,6 +681,7 @@ const struct coresight_ops tmc_etb_cs_ops = {
> >   const struct coresight_ops tmc_etf_cs_ops = {
> >   	.sink_ops	= &tmc_etf_sink_ops,
> >   	.link_ops	= &tmc_etf_link_ops,
> > +	.panic_ops	= &tmc_etf_sync_ops,
> >   };
> >   int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > index 041c428dd7cd..be1079e8fd64 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > @@ -1813,6 +1813,71 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev)
> >   	return 0;
> >   }
> > +static int tmc_panic_sync_etr(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;
> > +
> > +	if (!drvdata->etr_buf)
> > +		return 0;
> > +
> > +	/* Being in RESRV mode implies valid reserved memory as well */
> > +	if (drvdata->etr_buf->mode != ETR_MODE_RESRV)
> > +		return 0;
> > +
> > +	mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
> > +	mdata->valid = false;
> > +
> > +	CS_UNLOCK(drvdata->base);
> > +
> > +	/* Proceed only if ETR is enabled */
> > +	val = readl(drvdata->base + TMC_CTL);
> > +	if (!(val & TMC_CTL_CAPT_EN))
> > +		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__);
> > +		tmc_flush_and_stop(drvdata);
> > +	} else
> > +		tmc_wait_for_tmcready(drvdata);
> > +
> > +	/* Sync registers from hardware to metadata region */
> > +	mdata->size = csdev_access_relaxed_read32(csa, TMC_RSZ);
> > +	mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);
> > +	mdata->rrp = tmc_read_rrp(drvdata);
> > +	mdata->rwp = tmc_read_rwp(drvdata);
> > +	mdata->dba = tmc_read_dba(drvdata);
> > +	mdata->trc_paddr = drvdata->crash_tbuf.paddr;
> > +
> > +	/*
> > +	 * Make sure all previous writes are completed,
> > +	 * before we mark valid
> > +	 */
> > +	dsb(sy);
> > +	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__);
> > +out:
> > +	CS_UNLOCK(drvdata->base);
> > +
> > +	return 0;
> > +}
> > +
> >   static const struct coresight_ops_sink tmc_etr_sink_ops = {
> >   	.enable		= tmc_enable_etr_sink,
> >   	.disable	= tmc_disable_etr_sink,
> > @@ -1821,8 +1886,13 @@ static const struct coresight_ops_sink tmc_etr_sink_ops = {
> >   	.free_buffer	= tmc_free_etr_buffer,
> >   };
> > +static const struct coresight_ops_panic tmc_etr_sync_ops = {
> > +	.sync		= tmc_panic_sync_etr,
> > +};
> > +
> >   const struct coresight_ops tmc_etr_cs_ops = {
> >   	.sink_ops	= &tmc_etr_sink_ops,
> > +	.panic_ops	= &tmc_etr_sync_ops,
> >   };
> >   int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> > index c23dc9917ab9..35beee53584a 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc.h
> > +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> > @@ -12,6 +12,7 @@
> >   #include <linux/miscdevice.h>
> >   #include <linux/mutex.h>
> >   #include <linux/refcount.h>
> > +#include <linux/crc32.h>
> >   #define TMC_RSZ			0x004
> >   #define TMC_STS			0x00c
> > @@ -76,6 +77,9 @@
> >   #define TMC_AXICTL_AXCACHE_OS	(0xf << 2)
> >   #define TMC_AXICTL_ARCACHE_OS	(0xf << 16)
> > +/* TMC_FFSR - 0x300 */
> > +#define TMC_FFSR_FT_STOPPED	BIT(1)
> > +
> >   /* TMC_FFCR - 0x304 */
> >   #define TMC_FFCR_FLUSHMAN_BIT	6
> >   #define TMC_FFCR_EN_FMT		BIT(0)
> > @@ -131,6 +135,21 @@ enum tmc_mem_intf_width {
> >   #define CORESIGHT_SOC_600_ETR_CAPS	\
> >   	(TMC_ETR_SAVE_RESTORE | TMC_ETR_AXI_ARCACHE)
> 
> 
> 
> 
> > +/* TMC metadata region for ETR and ETF configurations */
> 
> I strongly think we should version the data layout to handle
> the future changes better (if at all).


> 
> 
> > +struct tmc_crash_metadata {
> > +	uint32_t crc32_mdata;	/* crc of metadata */
> > +	uint32_t crc32_tdata;	/* crc of tracedata */
> 
> 	uint32_t version;	/* Version of the structure = 1 */

Ack.

> 
> > +	uint32_t valid;         /* Indicate if this ETF/ETR was enabled */
> > +	uint32_t size;          /* Ram Size register */
> 
> Please save the size in bytes and also rename it :
> 
> 	uint32_t trace_size;	/* Trace size in Bytes */

The idea was to only take a register snapshot by kernel/firmware at the time of
panic/watchdog reset and thought that keeping that panic handler/watchdog reset
handler without any conversions would be better when external entities like firmware
is involved.

Please let me know if you still prefer adding this field.

> 
> 
> > +	uint32_t sts;           /* Status register */
> > +	uint32_t reserved32[3];
> > +	uint64_t rrp;           /* Ram Read pointer register */
> > +	uint64_t rwp;           /* Ram Write pointer register */
> 
> 
> 
> > +	uint64_t dba;		/* Data buffer address register */
> 
> Is this field useful ? And we store RRP/RWP relative to the DBA ? Could

RRP/RWP/DBA fields are just register snapshots.
Yes we use the DBA to calculate offsets. 
ie. etr_buf->offset = rrp - dba;


> we instead :
> 
> 1. Drop DBA
> 2. Store RRP and RWP as offsets from DBA. Or even convert them to the
> actual PADDRs relative to the trc_paddr.

Converting to actual PADDRs would be difficult in case of watchdog reset
scenarios where firmware is involved.

> 
> DBA could be a "DMA" Address and not necessarily the PA Address.
> We already have the trc_paddr below. (And for ETF, we already copy
> the buffer to the reserved buffer). So all the user needs to know
> is where the pointers are within the buffer. Having them relative
> to the "actual" location of the buffer is much useful than basing
> it on some unusable base address.

As pointed out above, the idea was to only take a register snapshot without
any conversions. Please let me know if you still prefer dropping the DBA.

> 
> > +	uint64_t trc_paddr;	/* Phys address of trace buffer */
> 
> s/trc/trace
> 
> Move RRP and RWP, after the above field.
> 
> For the sake of completeness, you are also missing :
> 
> 1) FFCR register => That tells you whether the Formatting was enabled or not
> (among other things) ? Though we always enable it, its good to
> capture it, if we ever decide to turn off the formatting.
> 
> 
> 2) MODE => Which mode was selected. Again, CIRCULAR_BUFFER for now,
> but lets seal it for the future, so that you can infer the trace buffer
> correctly with RRP/RWP.
> 
> 3) And may be FFSR, just in case the flush never completed and the
> data is not reliable ?

Sure will add the above three fields.

> 
> > +	uint64_t reserved64[3];
> > +};
> 
> 
> 
> 
> > +
> >   enum etr_mode {
> >   	ETR_MODE_FLAT,		/* Uses contiguous flat buffer */
> >   	ETR_MODE_ETR_SG,	/* Uses in-built TMC ETR SG mechanism */
> > @@ -204,6 +223,8 @@ struct tmc_resrv_buf {
> >    *		retention (after crash) only when ETR_MODE_RESRV buffer
> >    *		mode is enabled. Used by ETF for trace data retention
> >    *		(after crash) by default.
> > + * @crash_mdata: Reserved memory for storing tmc crash metadata.
> > + *		 Used by ETR/ETF.
> >    */
> >   struct tmc_drvdata {
> >   	struct clk		*pclk;
> > @@ -230,6 +251,7 @@ struct tmc_drvdata {
> >   	struct etr_buf		*sysfs_buf;
> >   	struct etr_buf		*perf_buf;
> >   	struct tmc_resrv_buf	crash_tbuf;
> > +	struct tmc_resrv_buf	crash_mdata;
> >   };
> >   struct etr_buf_operations {
> > @@ -352,11 +374,32 @@ static inline bool is_tmc_reserved_region_valid(struct device *dev)
> >   	struct tmc_drvdata *drvdata = dev_get_drvdata(dev);
> >   	if (drvdata->crash_tbuf.paddr &&
> > -		drvdata->crash_tbuf.size)
> > +		drvdata->crash_tbuf.size &&
> > +		drvdata->crash_mdata.paddr &&
> > +		drvdata->crash_mdata.size)
> 
> Why do we need to tie the "reserved" region to metdata region availability ?
> It is perfectly possible for another usecase
> to dedicate a buffer for trace and use it without metadata ?

Okay that can be reworked.

> 
> Suzuki
> 
> 
> >   		return true;
> >   	return false;
> >   }
> > +static inline uint32_t find_crash_metadata_crc(struct tmc_crash_metadata *md)
> > +{
> > +	unsigned long crc_size;
> > +
> > +	crc_size = sizeof(struct tmc_crash_metadata) -
> > +		offsetof(struct tmc_crash_metadata, crc32_tdata);
> > +	return crc32_le(0, (void *)&md->crc32_tdata, crc_size);
> > +}
> > +
> > +static inline uint32_t find_crash_tracedata_crc(struct tmc_drvdata *drvdata,
> > +						struct tmc_crash_metadata *md)
> > +{
> > +	unsigned long crc_size;
> > +
> > +	/* Take CRC of configured buffer size to keep it simple */
> > +	crc_size = md->size << 2;
> > +	return crc32_le(0, (void *)drvdata->crash_tbuf.vaddr, crc_size);
> > +}
> > +
> >   struct coresight_device *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata);
> >   void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu);
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Powered by blists - more mailing lists