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@xxxxxxxxxxx) wrote: > Hi Suzuki, > > On 2024-10-17 at 17:42:21, Linu Cherian (lcherian@xxxxxxxxxxx) wrote: > > Hi Suzuki, > > > > On 2024-10-01 at 22:13:12, Suzuki K Poulose (suzuki.poulose@xxxxxxx) 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@xxxxxxxxxxx> > > > > --- > > > > 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. > >