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

[Linu Cherian <lcherian@marvell.com>]

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.
>  
>