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 to change this dev_dbg instead of removing it. Have found
this quite useful to understand if the CTI trigger has fired.
>
> >
> > > + 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.
Would it be okay to convert this to dev_dbg? Helps to understand if the
panic call handler has run just in case if we want to debug if the metadata saving
happened or not.
>
> >
> > > +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 +675,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 +686,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 8bca5b36334a..8228d7aaa361 100644
> > > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > > @@ -1814,6 +1814,79 @@ 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;
> >
> > As earlier, drop the csa.
>
>
> Okay.
>
> >
> > > + mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
> > > +
> > > + 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;
> > > +
> > > + if (!tmc_has_reserved_buffer(drvdata) ||
> >
> > Do we need to check this again ? We wouldn't be in ETR_MODE_RESRV
> > otherwise, also indicated by the comment.
>
> Will drop.
>
> >
> > > + !tmc_has_crash_mdata_buffer(drvdata))
> > > + return 0;
> > > +
> > > + tmc_crashdata_set_invalid(drvdata);
> > > +
> > > + 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__);
> >
> > Drop the info
>
> Ack.
>
> >
> > > + 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->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);
> >
> > Please use raw reads, don't mix csa and raw reads.
>
> Ack.
>
> >
> > > + mdata->rrp = tmc_read_rrp(drvdata);
> > > + mdata->rwp = tmc_read_rwp(drvdata);
> > > + mdata->dba = tmc_read_dba(drvdata);
> > > + mdata->trace_paddr = drvdata->resrv_buf.paddr;
> > > + mdata->version = CS_CRASHDATA_VERSION;
> > > +
> > > + /*
> > > + * Make sure all previous writes are completed,
> > > + * before we mark valid
> > > + */
> > > + dsb(sy);
> >
> > Same as earlier, doesn't buy us much
>
> Will convert to dmb.
>
> >
> > > + 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__);
> >
> > Drop
> >
> > > +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,
> > > @@ -1822,8 +1895,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 d2261eddab71..75e504e51956 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)
> > > @@ -94,6 +98,9 @@
> > > #define TMC_AUTH_NSID_MASK GENMASK(1, 0)
> > > +/* Major version 1 Minor version 0 */
> > > +#define CS_CRASHDATA_VERSION (1 << 16)
> > > +
> > > enum tmc_config_type {
> > > TMC_CONFIG_TYPE_ETB,
> > > TMC_CONFIG_TYPE_ETR,
> > > @@ -131,6 +138,25 @@ 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 */
> > > +struct tmc_crash_metadata {
> > > + uint32_t crc32_mdata; /* crc of metadata */
> > > + uint32_t crc32_tdata; /* crc of tracedata */
> > > + uint32_t version; /* 31:16 Major version, 15:0 Minor version */
> > > + uint32_t valid; /* Indicate if this ETF/ETR was enabled */
> > > + uint32_t size; /* Ram Size register */
> >
> > Please could you not keep this "plain bytes" ? Or rename the field to
> > tmc_ram_size if we want to stick to TMC RAM SIZE register. It is very
> > easy to confuse it with "normal" bytes.
>
> Okay. Will change the name.
>
> >
> > > + uint32_t sts; /* Status register */
> >
> > tmc_sts
> >
> > > + uint32_t mode; /* Mode register */
> >
> > tmc_mode
> >
> > This doesn't look packed. Please could you add a padding here to make sure
> > the fields are 64bit aligned ?
> >
> > > + uint64_t ffcr; /* Formatter and flush control register */
> >
> > tmc_ffcr
> >
> > > + uint64_t ffsr; /* Formatter and flush status register */
> >
> > tmc_ffsr
> >
> >
> > Also, why are they both 64bit ? They are all 32bit for sure ?
> >
> > > + uint32_t reserved32[3];
> >
> > Why do we have reserved bits here ? They should be near the 32bit fields.
>
> Have added them for the sake of future extensions, just in case if we
> wish to add additional registers without changing the overall metadata
> size.
>
>
> >
> > I think, once you fix the type of ffcr and ffsr things, everything will
> > be in order.
>
>
> Okay will fix.
>
> >
> >
> > > + uint64_t rrp; /* Ram Read pointer register */
> > > + uint64_t rwp; /* Ram Write pointer register */
> > > + uint64_t dba; /* Data buffer address register */
> > > + uint64_t trace_paddr; /* Phys address of trace buffer */
> > > + uint64_t reserved64[3];
> > > +};
> >
> >
> > Suzuki
> >
> >
> > > +
> > > enum etr_mode {
> > > ETR_MODE_FLAT, /* Uses contiguous flat buffer */
> > > ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */
> > > @@ -205,6 +231,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;
> > > @@ -231,6 +259,7 @@ struct tmc_drvdata {
> > > struct etr_buf *sysfs_buf;
> > > struct etr_buf *perf_buf;
> > > struct tmc_resrv_buf resrv_buf;
> > > + struct tmc_resrv_buf crash_mdata;
> > > };
> > > struct etr_buf_operations {
> > > @@ -356,6 +385,43 @@ static inline bool tmc_has_reserved_buffer(struct tmc_drvdata *drvdata)
> > > return false;
> > > }
> > > +static inline bool tmc_has_crash_mdata_buffer(struct tmc_drvdata *drvdata)
> > > +{
> > > + if (drvdata->crash_mdata.vaddr &&
> > > + drvdata->crash_mdata.size)
> > > + return true;
> > > + return false;
> > > +}
> > > +
> > > +static inline void tmc_crashdata_set_invalid(struct tmc_drvdata *drvdata)
> > > +{
> > > + struct tmc_crash_metadata *mdata;
> > > +
> > > + mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
> > > +
> > > + if (tmc_has_crash_mdata_buffer(drvdata))
> > > + mdata->valid = 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->resrv_buf.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);
> >
> >
>
