On 2024-06-10 at 18:32:33, Suzuki K Poulose (suzuki.poulose@xxxxxxx) 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@xxxxxxxxxxx>
> > Reviewed-by: James Clark <james.clark@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
