-----Original Message----- From: James Clark james.clark@arm.com Sent: Wednesday, August 16, 2023 10:36 PM To: Linu Cherian lcherian@marvell.com; suzuki.poulose@arm.com; mike.leach@linaro.org; mathieu.poirier@linaro.org Cc: coresight@lists.linaro.org; Anil Kumar Reddy H areddy3@marvell.com; Tanmay Jagdale tanmay@marvell.com; George Cherian gcherian@marvell.com Subject: [EXT] Re: [RFC PATCH v2 1/7] dt-bindings: arm: coresight-tmc: Add "memory-region" property

External Email

---

On 13/07/2023 14:47, Linu Cherian wrote:

...

memory-region 0: Reserved trace buffer memory

TMC ETR: When available, use this reserved memory region for trace data capture. Same region is used for trace data retention after a panic or watchdog reset.

TMC ETF: When available, use this reserved memory region for trace data retention synced from internal SRAM after a panic or watchdog reset.

memory-region 1: Reserved meta data memory

TMC ETR, ETF: When available, use this memory for register snapshot retention synced from hardware registers after a panic or watchdog reset.

Signed-off-by: Linu Cherian lcherian@marvell.com

.../devicetree/bindings/arm/arm,coresight-tmc.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml index cb8dceaca70e..10da9331c165 100644 --- a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml +++ b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml @@ -101,6 +101,13 @@ properties: and ETF configurations. $ref: /schemas/graph.yaml#/properties/port

• memory-region:
• items:

•  - description: Reserved trace buffer memory. Used for ETR and ETF
  

•      configurations.
  

•  - description: Reserved meta data memory. Used for ETR and ETF
  

•      configurations.
  

I don't see why you couldn't take the more detailed explanation from the commit message and put it here, it would be more helpful.

Ack.

...

required:

• compatible
• reg

@@ -115,6 +122,8 @@ examples: etr@20070000 { compatible = "arm,coresight-tmc", "arm,primecell"; reg = <0x20070000 0x1000>;

•    memory-region = <&etr_trace_mem_reserved>,
  

•                   <&etr_mdata_mem_reserved>;
  
     clocks = <&oscclk6a>;
     clock-names = "apb_pclk";
  

On 13/07/2023 14:47, Linu Cherian wrote:

Add support to use reserved memory for coresight ETR trace buffer.

Introduce a new ETR buffer mode called ETR_MODE_RESRV, which gets enabled when ETR device tree node is supplied with a valid reserved memory region.

Signed-off-by: Anil Kumar Reddy areddy3@marvell.com Signed-off-by: Linu Cherian lcherian@marvell.com

---

.../hwtracing/coresight/coresight-tmc-core.c | 57 ++++++++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 67 ++++++++++++++++++- drivers/hwtracing/coresight/coresight-tmc.h | 16 +++++ 3 files changed, 139 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index c106d142e632..b0374ae007d2 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -21,6 +21,7 @@ #include <linux/spinlock.h> #include <linux/pm_runtime.h> #include <linux/of.h> +#include <linux/of_address.h> #include <linux/coresight.h> #include <linux/amba/bus.h> @@ -362,6 +363,54 @@ static inline bool tmc_etr_has_non_secure_access(struct tmc_drvdata *drvdata) return (auth & TMC_AUTH_NSID_MASK) == 0x3; } +static inline bool is_tmc_reserved_region_valid(struct device *dev) +{

• struct tmc_drvdata *drvdata = dev_get_drvdata(dev);
• if (drvdata->resrv_buf.paddr &&

• drvdata->resrv_buf.size)
  

• return true;
  

• return false;

+}

+static void tmc_get_reserved_region(struct device *parent) +{

• struct tmc_drvdata *drvdata = dev_get_drvdata(parent);
• struct device_node *node;
• struct resource res;
• int rc;
• node = of_parse_phandle(parent->of_node, "memory-region", 0);
• if (!node) {

• dev_dbg(parent, "No reserved trace buffer specified\n");
  

• goto out;
  

• }
• rc = of_address_to_resource(node, 0, &res);
• of_node_put(node);
• if (rc || res.start == 0 || resource_size(&res) == 0) {

• dev_err(parent, "Reserved trace buffer memory is invalid\n");
  

• goto out;
  

• }
• drvdata->resrv_buf.vaddr = memremap(res.start,

• 				resource_size(&res),
  

• 				MEMREMAP_WC);
  

• if (IS_ERR(drvdata->resrv_buf.vaddr)) {

• dev_err(parent, "Reserved trace buffer mapping failed\n");
  

• rc = PTR_ERR(drvdata->resrv_buf.vaddr);
  

• goto out;
  

• }
• drvdata->resrv_buf.paddr = res.start;
• drvdata->resrv_buf.size = resource_size(&res);
• /* Size of contiguous buffer space for TMC ETR */
• drvdata->size = drvdata->resrv_buf.size;

+out:

• return;

+}

/* Detect and initialise the capabilities of a TMC ETR */ static int tmc_etr_setup_caps(struct device *parent, u32 devid, void *dev_caps) { @@ -378,6 +427,12 @@ static int tmc_etr_setup_caps(struct device *parent, u32 devid, void *dev_caps) if (!(devid & TMC_DEVID_NOSCAT) && tmc_etr_can_use_sg(parent)) tmc_etr_set_cap(drvdata, TMC_ETR_SG);

• /* Get reserved memory region if specified and

• * set capability to use reserved memory for trace buffer.
  

• */
  

• if (is_tmc_reserved_region_valid(parent))

• tmc_etr_set_cap(drvdata, TMC_ETR_RESRV_MEM);
  

• /* Check if the AXI address width is available */ if (devid & TMC_DEVID_AXIAW_VALID) dma_mask = ((devid >> TMC_DEVID_AXIAW_SHIFT) &

@@ -473,6 +528,8 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) drvdata->size = readl_relaxed(drvdata->base + TMC_RSZ) * 4; }

• tmc_get_reserved_region(dev);
• desc.dev = dev; desc.groups = coresight_tmc_groups;

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index eaa296ced167..857c77d867c5 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -686,6 +686,61 @@ static const struct etr_buf_operations etr_flat_buf_ops = { .get_data = tmc_etr_get_data_flat_buf, }; +/*

• • tmc_etr_alloc_resrv_buf: Allocate a contiguous DMA buffer from reserved region.
• */

+static int tmc_etr_alloc_resrv_buf(struct tmc_drvdata *drvdata,

• 		  struct etr_buf *etr_buf, int node,
  

• 		  void **pages)
  

+{

• struct etr_flat_buf *resrv_buf;
• struct device *real_dev = drvdata->csdev->dev.parent;
• /* We cannot reuse existing pages for resrv buf */
• if (pages)

• return -EINVAL;
  

• resrv_buf = kzalloc(sizeof(*resrv_buf), GFP_KERNEL);
• if (!resrv_buf)

• return -ENOMEM;
  

• resrv_buf->daddr = dma_map_resource(real_dev, drvdata->resrv_buf.paddr,

• 			   etr_buf->size, DMA_FROM_DEVICE, 0);
  

• if (dma_mapping_error(real_dev, resrv_buf->daddr)) {

• dev_err(real_dev, "failed to map source buffer address\n");
  

• kfree(resrv_buf);
  

• return -ENOMEM;
  

• }
• resrv_buf->vaddr = drvdata->resrv_buf.vaddr;
• resrv_buf->size = etr_buf->size;
• resrv_buf->dev = &drvdata->csdev->dev;
• etr_buf->hwaddr = resrv_buf->daddr;
• etr_buf->mode = ETR_MODE_RESRV;
• etr_buf->private = resrv_buf;
• return 0;

+}

+static void tmc_etr_free_resrv_buf(struct etr_buf *etr_buf) +{

• struct etr_flat_buf *resrv_buf = etr_buf->private;
• if (resrv_buf && resrv_buf->daddr) {

• struct device *real_dev = resrv_buf->dev->parent;
  

• dma_unmap_resource(real_dev, resrv_buf->daddr,
  

• 		resrv_buf->size, DMA_FROM_DEVICE, 0);
  

• }
• kfree(resrv_buf);

+}

+static const struct etr_buf_operations etr_resrv_buf_ops = {

• .alloc = tmc_etr_alloc_resrv_buf,
• .free = tmc_etr_free_resrv_buf,
• .sync = tmc_etr_sync_flat_buf,
• .get_data = tmc_etr_get_data_flat_buf,

+};

/*

• tmc_etr_alloc_sg_buf: Allocate an SG buf @etr_buf. Setup the parameters
• appropriately.

@@ -813,6 +868,7 @@ static const struct etr_buf_operations *etr_buf_ops[] = { [ETR_MODE_FLAT] = &etr_flat_buf_ops, [ETR_MODE_ETR_SG] = &etr_sg_buf_ops, [ETR_MODE_CATU] = NULL,

• [ETR_MODE_RESRV] = &etr_resrv_buf_ops

}; void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu) @@ -838,6 +894,7 @@ static inline int tmc_etr_mode_alloc_buf(int mode, case ETR_MODE_FLAT: case ETR_MODE_ETR_SG: case ETR_MODE_CATU:

• case ETR_MODE_RESRV: if (etr_buf_ops[mode] && etr_buf_ops[mode]->alloc) rc = etr_buf_ops[mode]->alloc(drvdata, etr_buf, node, pages);

@@ -862,7 +919,7 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, int node, void **pages) { int rc = -ENOMEM;

• bool has_etr_sg, has_iommu;

• bool has_etr_sg, has_iommu, has_etr_resrv_mem; bool has_sg, has_catu; struct etr_buf *etr_buf; struct device *dev = &drvdata->csdev->dev;

@@ -870,6 +927,7 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); has_iommu = iommu_get_domain_for_dev(dev->parent); has_catu = !!tmc_etr_get_catu_device(drvdata);

• has_etr_resrv_mem = tmc_etr_has_cap(drvdata, TMC_ETR_RESRV_MEM);

has_sg = has_catu || has_etr_sg; @@ -891,6 +949,12 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, * Fallback to available mechanisms. * */

• if (has_etr_resrv_mem) {

• rc = tmc_etr_mode_alloc_buf(ETR_MODE_RESRV, drvdata,
  

• 			    etr_buf, node, pages);
  

• if (!rc)
  

• 	goto done;
  

• }

Does this mean that you can _only_ use reserved memory if it's specified in the device tree? I'm not sure what all the use cases might be, but would it be better if you could still use Coresight normally if you don't want the panic support? For example if the reserved size was smaller than a user wants.

That also makes me wonder if there should be some subset of CS_MODE_SYSFS, where it's sysfs mode, but it's also using the reserved buffer, so it supports the panic save stuff. Otherwise you could be in sysfs mode but haven't written anything into the reserved buffer so setting the valid panic dump flag would be the wrong thing to do.

if (!pages && (!has_sg || has_iommu || size < SZ_1M)) rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, @@ -901,6 +965,7 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, if (rc && has_catu) rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, etr_buf, node, pages); +done: if (rc) { kfree(etr_buf); return ERR_PTR(rc); diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 01c0382a29c0..3c344a3a8953 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -126,6 +126,8 @@ enum tmc_mem_intf_width {

• so we have to rely on PID of the IP to detect the functionality.

*/ #define TMC_ETR_SAVE_RESTORE (0x1U << 2) +/* TMC ETR to use reserved memory for trace buffer*/ +#define TMC_ETR_RESRV_MEM (0x1U << 3) /* Coresight SoC-600 TMC-ETR unadvertised capabilities */ #define CORESIGHT_SOC_600_ETR_CAPS \ @@ -135,6 +137,7 @@ enum etr_mode { ETR_MODE_FLAT, /* Uses contiguous flat buffer */ ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */ ETR_MODE_CATU, /* Use SG mechanism in CATU */

• ETR_MODE_RESRV, /* Use reserved region contiguous buffer */

}; struct etr_buf_operations; @@ -163,6 +166,17 @@ struct etr_buf { void *private; }; +/**

• • @paddr : Start address of reserved memory region.
• • @vaddr : Corresponding CPU virtual address.
• • @size : Size of reserved memory region.
• */

+struct tmc_resrv_buf {

• phys_addr_t paddr;
• void *vaddr;
• size_t size;

+};

/**

• struct tmc_drvdata - specifics associated to an TMC component
• @base: memory mapped base address for this component.

@@ -187,6 +201,7 @@ struct etr_buf {

• @idr_mutex: Access serialisation for idr.
• @sysfs_buf: SYSFS buffer for ETR.
• @perf_buf: PERF buffer for ETR.

• • @resrv_buf: Reserved Memory for trace data buffer. Used by ETR/ETF.
  */

struct tmc_drvdata { void __iomem *base; @@ -211,6 +226,7 @@ struct tmc_drvdata { struct mutex idr_mutex; struct etr_buf *sysfs_buf; struct etr_buf *perf_buf;

• struct tmc_resrv_buf resrv_buf;

}; struct etr_buf_operations {

Hi James,

-----Original Message----- From: James Clark james.clark@arm.com Sent: Thursday, August 17, 2023 7:21 PM To: Linu Cherian lcherian@marvell.com; suzuki.poulose@arm.com; mike.leach@linaro.org; mathieu.poirier@linaro.org Cc: coresight@lists.linaro.org; Anil Kumar Reddy H areddy3@marvell.com; Tanmay Jagdale tanmay@marvell.com; George Cherian gcherian@marvell.com Subject: [EXT] Re: [RFC PATCH v2 3/7] coresight: core: Add provision for panic callbacks

External Email

---

On 13/07/2023 14:47, Linu Cherian wrote:

...

Panic callback handlers allows coresight device drivers to sync relevant trace data and trace metadata to reserved memory regions so that they can be retrieved later in the subsequent boot or in the crashdump kernel.

Signed-off-by: Linu Cherian lcherian@marvell.com

drivers/hwtracing/coresight/coresight-core.c | 30

++++++++++++++++++++

...

include/linux/coresight.h | 11 +++++++ 2 files changed, 41 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index d3bf82c0de1d..aa29a7d18518 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -19,6 +19,7 @@ #include <linux/of_platform.h> #include <linux/delay.h> #include <linux/pm_runtime.h> +#include <linux/panic_notifier.h>

#include "coresight-etm-perf.h" #include "coresight-priv.h" @@ -1765,6 +1766,31 @@ struct bus_type coresight_bustype = { .name = "coresight", };

+static int coresight_panic_sync(struct device *dev, void *data) {

• struct coresight_device *csdev = container_of(dev, struct

+coresight_device, dev);

• /* Run through panic sync handlers for all enabled devices */
• if (csdev->enable && panic_ops(csdev))

• panic_ops(csdev)->sync(csdev);
  

• return 0;

+}

+static int coresight_panic_cb(struct notifier_block *self,

• 	       unsigned long v, void *p)
  

+{

• bus_for_each_dev(&coresight_bustype, NULL, NULL,

• 		 coresight_panic_sync);
  

• return 0;

+}

+static struct notifier_block coresight_notifier = {

• .notifier_call = coresight_panic_cb, };

static int __init coresight_init(void) { int ret; @@ -1777,6 +1803,10 @@ static int __init coresight_init(void) if (ret) goto exit_bus_unregister;

• /* Register function to be called for panic */
• ret = atomic_notifier_chain_register(&panic_notifier_list,

• 			     &coresight_notifier);
  

I think this needs to be unregistered on unload, or not done on every load, otherwise doing rmmod and then insmod gives you an error:

[ 3273.744765] ------------[ cut here ]------------ [ 3273.744775] notifier callback coresight_panic_cb [coresight] already registered [ 3273.744846] WARNING: CPU: 3 PID: 4282 at kernel/notifier.c:32 notifier_chain_register+0x140/0x248 [ 3273.744862] Modules linked in: coresight(+) dm_mod xt_CHECKSUM xt_MASQUERADE xt_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp ip6table_mangle ip6table_nat iptable_mangle iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c ip6table_filter ip6_tables iptable_filter bridge stp llc arm_spe_pmu ip_tables x_tables ipv6 xhci_pci xhci_pci_renesas r8169 [last unloaded: coresight] [ 3273.744954] CPU: 3 PID: 4282 Comm: modprobe Not tainted 6.5.0-rc3+ #145 [ 3273.744962] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 3273.744970] pc : notifier_chain_register+0x140/0x248 [ 3273.744977] lr : notifier_chain_register+0x13c/0x248 [ 3273.744984] sp : ffff8000893e7780 [ 3273.744988] x29: ffff8000893e7780 x28: ffff800085d2c273 x27: 0000000000000009 [ 3273.745000] x26: ffff80007bdc4040 x25: ffff80007bdb22d8 x24: 0000000000000000 [ 3273.745012] x23: ffff80007bdadde0 x22: ffff80007bdaddf0 x21: 0000000000000000 [ 3273.745024] x20: ffff800082a1f848 x19: ffff80007bdadde0 x18: ffff00837dff7400 [ 3273.745036] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000001 [ 3273.745048] x14: 1fffe0105dad2022 x13: 0000000000000000 x12: 0000000000000000 [ 3273.745060] x11: ffff60105dad2023 x10: 1fffe01008a07001 x9 : 9df4d03567f7fe00 [ 3273.745072] x8 : 9df4d03567f7fe00 x7 : 0000000000000001 x6 : 0000000000000001 [ 3273.745084] x5 : ffff8000893e7478 x4 : ffff80008241eac0 x3 : ffff8000803322fc [ 3273.745096] x2 : 0000000000000001 x1 : 0000000000000004 x0 : 00000000ffffffef [ 3273.745107] Call trace: [ 3273.745111] notifier_chain_register+0x140/0x248 [ 3273.745118] atomic_notifier_chain_register+0x40/0x70 [ 3273.745125] init_module+0x74/0x90 [coresight] [ 3273.745184] do_one_initcall+0x11c/0x470 [ 3273.745191] do_init_module+0x114/0x338 [ 3273.745198] load_module+0x2248/0x2478 [ 3273.745206] __arm64_sys_finit_module+0x2b8/0x3d8 [ 3273.745213] invoke_syscall+0x60/0x188 [ 3273.745221] el0_svc_common+0x110/0x158 [ 3273.745228] do_el0_svc+0x4c/0xe0 [ 3273.745235] el0_svc+0x44/0xb8 [ 3273.745242] el0t_64_sync_handler+0x84/0x100 [ 3273.745248] el0t_64_sync+0x190/0x198 [ 3273.745254] irq event stamp: 0 [ 3273.745257] hardirqs last enabled at (0): [<0000000000000000>] 0x0 [ 3273.745265] hardirqs last disabled at (0): [<ffff8000800ca374>] copy_process+0x994/0x18f0 [ 3273.745272] softirqs last enabled at (0): [<ffff8000800ca38c>] copy_process+0x9ac/0x18f0 [ 3273.745279] softirqs last disabled at (0): [<0000000000000000>] 0x0 [ 3273.745284] ---[ end trace 0000000000000000 ]---

Ack. Will fix.

...

/* initialise the coresight syscfg API */ ret = cscfg_init(); if (!ret) diff --git a/include/linux/coresight.h b/include/linux/coresight.h index f19a47b9bb5a..8831df24733a 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -277,6 +277,7 @@ static struct coresight_dev_list (var) = {

	\

...

#define link_ops(csdev) csdev->ops->link_ops #define helper_ops(csdev) csdev->ops->helper_ops #define ect_ops(csdev) csdev->ops->ect_ops +#define panic_ops(csdev) csdev->ops->panic_ops

/**

• struct coresight_ops_sink - basic operations for a sink @@ -351,12

+352,22 @@ struct coresight_ops_ect { int (*disable)(struct coresight_device *csdev); };

+/**

• • struct coresight_ops_panic - Generic device ops for panic handing
• • @sync : Sync the device register state/trace data
• */

+struct coresight_ops_panic {

• int (*sync)(struct coresight_device *csdev); };

struct coresight_ops { const struct coresight_ops_sink *sink_ops; const struct coresight_ops_link *link_ops; const struct coresight_ops_source *source_ops; const struct coresight_ops_helper *helper_ops; const struct coresight_ops_ect *ect_ops;

• const struct coresight_ops_panic *panic_ops;

};

#if IS_ENABLED(CONFIG_CORESIGHT)

On 13/07/2023 14:47, 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@marvell.com

.../hwtracing/coresight/coresight-tmc-core.c | 30 +++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 56 +++++++++++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 55 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-tmc.h | 18 ++++++ 4 files changed, 158 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index b0374ae007d2..67a4bc6f4bff 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -368,7 +368,9 @@ static inline bool is_tmc_reserved_region_valid(struct device *dev) struct tmc_drvdata *drvdata = dev_get_drvdata(dev); if (drvdata->resrv_buf.paddr &&

• drvdata->resrv_buf.size)
  

• drvdata->resrv_buf.size &&
  

• drvdata->metadata.paddr &&
  

• drvdata->metadata.size)
  

  return true; return false;

} @@ -407,6 +409,32 @@ static void tmc_get_reserved_region(struct device *parent) /* Size of contiguous buffer space for TMC ETR */ drvdata->size = drvdata->resrv_buf.size;

• /* Metadata region */
• node = of_parse_phandle(parent->of_node, "memory-region", 1);
• if (!node) {

• dev_dbg(parent, "No metadata memory-region specified\n");
  

• goto out;
  

• }
• 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");
  

• goto out;
  

• }
• drvdata->metadata.vaddr = memremap(res.start,

• 			       resource_size(&res),
  

• 			       MEMREMAP_WC);
  

• if (IS_ERR(drvdata->metadata.vaddr)) {

• dev_err(parent, "Metadata memory mapping failed\n");
  

• rc = PTR_ERR(drvdata->metadata.vaddr);
  

• goto out;
  

• }
• drvdata->metadata.paddr = res.start;
• drvdata->metadata.size = resource_size(&res);

out: return; } diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index 0ab1f73c2d06..63f59ea1508e 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -586,6 +586,57 @@ 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 tmc_register_snapshot *tmc;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• /* Make sure we have valid reserved memory */
• if (!drvdata->metadata.vaddr || !drvdata->resrv_buf.vaddr)

• return 0;
  

• tmc = (struct tmc_register_snapshot *)drvdata->metadata.vaddr;
• tmc->valid = 0x0;
• 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;
  

• tmc_flush_and_stop(drvdata);
• /* Sync registers from hardware to metadata region */
• tmc->sts = readl(drvdata->base + TMC_STS);
• tmc->trc_addrhi = drvdata->resrv_buf.paddr >> 32;
• tmc->trc_addr = drvdata->resrv_buf.paddr & 0xffffffff;
• /* Sync Internal SRAM to reserved trace buffer region */
• tmc_etb_dump_hw(drvdata);
• memcpy(drvdata->resrv_buf.vaddr, drvdata->buf, drvdata->len);
• tmc->size = drvdata->len;
• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);
• tmc->valid = 0x1;
• 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, @@ -599,6 +650,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, }; @@ -606,6 +661,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 857c77d867c5..a99b3e9fb9ec 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1784,8 +1784,63 @@ static const struct coresight_ops_sink tmc_etr_sink_ops = { .free_buffer = tmc_free_etr_buffer, }; +static int tmc_etr_panic_sync(struct coresight_device *csdev) +{

• u32 val;
• struct tmc_register_snapshot *tmc;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• /* Make sure we have valid reserved memory */
• if (!drvdata->metadata.vaddr || !drvdata->resrv_buf.vaddr)

• return 0;
  

• tmc = (struct tmc_register_snapshot *)drvdata->metadata.vaddr;
• tmc->valid = 0x0;
• CS_UNLOCK(drvdata->base);
• /* Proceed only if ETR is enabled */
• val = readl(drvdata->base + TMC_CTL);
• if (!(val & TMC_CTL_CAPT_EN))

• goto out;
  

• tmc_flush_and_stop(drvdata);
• /* Sync registers from hardware to metadata region */
• tmc->size = readl(drvdata->base + TMC_RSZ);
• tmc->rrphi = readl(drvdata->base + TMC_RRPHI);
• tmc->rrp = readl(drvdata->base + TMC_RRP);
• tmc->rwphi = readl(drvdata->base + TMC_RWPHI);
• tmc->rwp = readl(drvdata->base + TMC_RWP);
• tmc->sts = readl(drvdata->base + TMC_STS);
• tmc->trc_addrhi = drvdata->resrv_buf.paddr >> 32;
• tmc->trc_addr = drvdata->resrv_buf.paddr & 0xffffffff;
• tmc->dba = readl(drvdata->base + TMC_DBALO);
• tmc->dbahi = readl(drvdata->base + TMC_DBAHI);

Can you just store a 64bit value and use tmc_read_dba()? There are a couple of 32bit pairs that are read and restored manually when there are already some utility functions for that.

There is also the new register access abstraction:

struct csdev_access *csa = &drvdata->csdev.access; tmc->size = csdev_access_relaxed_read32(csa, TMC_RSZ)

• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);
• tmc->valid = 0x1;
• tmc_disable_hw(drvdata);
• dev_info(&csdev->dev, "%s: success\n", __func__);

+out:

• CS_UNLOCK(drvdata->base);
• return 0;

+}

+static const struct coresight_ops_panic tmc_etr_sync_ops = {

• .sync = tmc_etr_panic_sync,

+};

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 3c344a3a8953..8ebf624b26d9 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -133,6 +133,22 @@ 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_register_snapshot {

• uint32_t valid; /* Indicate if this ETF/ETR was enabled */
• uint32_t size; /* Size of trace data */
• uint32_t rrphi; /* Read pointer high address bits */
• uint32_t rrp; /* Read pointer */
• uint32_t rwphi; /* Write pointer high address bits */
• uint32_t rwp; /* Write pointer */
• uint32_t sts; /* Status register */
• uint32_t trc_addrhi; /* Phys high address bits of trace buffer */
• uint32_t trc_addr; /* Phys address of trace buffer */
• uint32_t dbahi; /* Data buffer high address bits */
• uint32_t dba; /* Data buffer address */
• uint32_t reserved[5];

+};

enum etr_mode { ETR_MODE_FLAT, /* Uses contiguous flat buffer */ ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */ @@ -202,6 +218,7 @@ struct tmc_resrv_buf {

• @sysfs_buf: SYSFS buffer for ETR.
• @perf_buf: PERF buffer for ETR.
• @resrv_buf: Reserved Memory for trace data buffer. Used by ETR/ETF.

• • @metadata: Reserved memory for metadata. Used by ETR/ETF.
  */

struct tmc_drvdata { void __iomem *base; @@ -227,6 +244,7 @@ struct tmc_drvdata { struct etr_buf *sysfs_buf; struct etr_buf *perf_buf; struct tmc_resrv_buf resrv_buf;

• struct tmc_resrv_buf metadata;

}; struct etr_buf_operations {

Hi James,

-----Original Message----- From: James Clark james.clark@arm.com Sent: Wednesday, August 16, 2023 10:35 PM To: Linu Cherian lcherian@marvell.com; suzuki.poulose@arm.com; mike.leach@linaro.org; mathieu.poirier@linaro.org Cc: coresight@lists.linaro.org; Anil Kumar Reddy H areddy3@marvell.com; Tanmay Jagdale tanmay@marvell.com; George Cherian gcherian@marvell.com Subject: [EXT] Re: [RFC PATCH v2 5/7] coresight: tmc: Add support for reading tracedata from previous boot

External Email

---

On 13/07/2023 14:47, Linu Cherian wrote:

...

• Introduce a new mode CS_MODE_READ_PREVBOOT for reading

tracedata

...

captured in previous boot.

• Add special handlers for preparing ETR/ETF for this special mode

• User can read the trace data as below

  For example, for reading trace data from tmc_etf sink

  1. cd /sys/bus/coresight/devices/tmc_etfXX/

  2. Change mode to READ_PREVBOOT

     #echo 1 > read_prevboot

  3. Dump trace buffer data to a file,

     #dd if=/dev/tmc_etrXX of=~/cstrace.bin

  4. Reset back to normal mode

     #echo 0 > read_prevboot

Signed-off-by: Anil Kumar Reddy areddy3@marvell.com Signed-off-by: Tanmay Jagdale tanmay@marvell.com Signed-off-by: Linu Cherian lcherian@marvell.com

---

drivers/hwtracing/coresight/coresight-priv.h | 1 + .../hwtracing/coresight/coresight-tmc-core.c | 38 ++++- .../hwtracing/coresight/coresight-tmc-etf.c | 85 ++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 154 +++++++++++++++++- drivers/hwtracing/coresight/coresight-tmc.h | 2 + 5 files changed, 278 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index 595ce5862056..2b8b83e72849 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -86,6 +86,7 @@ enum cs_mode { CS_MODE_DISABLED, CS_MODE_SYSFS, CS_MODE_PERF,

• CS_MODE_READ_PREVBOOT, /* Trace data from previous boot */

This causes an unhandled case error in etm4_disable() if you have CONFIG_WERROR=y

Will fix.

...

};

/** diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 67a4bc6f4bff..2f97bb4f5760 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -152,6 +152,10 @@ static int tmc_open(struct inode *inode, struct file

*file)

...

struct tmc_drvdata *drvdata = container_of(file->private_data, struct tmc_drvdata,

miscdev);

...

• /* Advertise if we are opening with a special mode */
• if (drvdata->mode == CS_MODE_READ_PREVBOOT)

• dev_info(&drvdata->csdev->dev, "TMC read mode for
  

previous

...

+boot\n");

I think dev_info might be too high a log level for this because it's something that's supposed to happen. You could do dev_info_once, but that's probably not that useful. So dev_dbg might be better. There are some other new dev_infos that have been added in other commits that also look like they might print multiple times.

Okay will change this. Will revisit the other dev_dbg cases.

...

ret = tmc_read_prepare(drvdata); if (ret) return ret; @@ -330,9 +334,40 @@ static ssize_t buffer_size_store(struct device *dev,

static DEVICE_ATTR_RW(buffer_size);

+static ssize_t read_prevboot_show(struct device *dev,

• 		struct device_attribute *attr, char *buf) {
  

• struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent);
• return sprintf(buf, "%#x\n", drvdata->size); }

Should this not show drvdata->mode == CS_MODE_READ_PREVBOOT, rather than the size of the buffer? The size is already available through buffer_size, but there is no way to get the mode that it's in.

Yeah. Will fix this.

...

+static ssize_t read_prevboot_store(struct device *dev,

• 		 struct device_attribute *attr,
  

• 		 const char *buf, size_t size)
  

+{

• int ret;
• unsigned long val;
• struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent);
• ret = kstrtoul(buf, 0, &val);
• if (ret)

• return ret;
  

• if (val)

• drvdata->mode = CS_MODE_READ_PREVBOOT;
  

• else

• drvdata->mode = CS_MODE_DISABLED;
  

I saw there were some more thorough checks for changing the mode, for example etb_enable_sysfs(). It might be worth doing something similar here or making a re-usable mode changing function that does the appropriate checks, plus locking.

Ack. Will ensure necessary checks.

...

• return size;

+}

+static DEVICE_ATTR_RW(read_prevboot);

static struct attribute *coresight_tmc_attrs[] = { &dev_attr_trigger_cntr.attr, &dev_attr_buffer_size.attr,

• &dev_attr_read_prevboot.attr, NULL,

};

@@ -632,7 +667,8 @@ static void tmc_shutdown(struct amba_device

*adev)

...

spin_lock_irqsave(&drvdata->spinlock, flags);

• if (drvdata->mode == CS_MODE_DISABLED)

• if (drvdata->mode == CS_MODE_DISABLED ||

•    drvdata->mode == CS_MODE_READ_PREVBOOT)
  

  goto out;

  if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) diff --git

a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index 63f59ea1508e..434b1586af54 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -664,6 +664,85 @@ const struct coresight_ops tmc_etf_cs_ops = { .panic_ops = &tmc_etf_sync_ops, };

+static int tmc_etb_setup_prevboot_buf(struct tmc_drvdata *drvdata) {

• u64 trace_addr;
• struct tmc_register_snapshot *reg_ptr;
• reg_ptr = drvdata->metadata.vaddr;
• trace_addr = reg_ptr->trc_addr | ((u64)reg_ptr->trc_addrhi << 32);
• drvdata->buf = memremap(trace_addr, reg_ptr->size,

MEMREMAP_WC);

...

• if (IS_ERR(drvdata->buf))

• return -ENOMEM;
  

• drvdata->len = reg_ptr->size;
• return 0;

+}

+static void tmc_etb_free_prevboot_buf(struct tmc_drvdata *drvdata) {

• void *buf = drvdata->buf;
• if (!buf)

• return;
  

• memunmap(buf);
• drvdata->buf = NULL;

+}

+static int tmc_etb_prepare_prevboot(struct tmc_drvdata *drvdata) {

• int ret = 0;
• unsigned long flags;
• struct tmc_register_snapshot *reg_ptr;
• spin_lock_irqsave(&drvdata->spinlock, flags);
• if (drvdata->reading) {

• ret = -EBUSY;
  

• goto out;
  

• }
• if (!drvdata->metadata.vaddr) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• reg_ptr = drvdata->metadata.vaddr;
• if (!reg_ptr->valid) {

• dev_err(&drvdata->csdev->dev,
  

• 	"Invalid metadata captured from previous boot\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }
• ret = tmc_etb_setup_prevboot_buf(drvdata);
• if (ret)

• goto out;
  

• if (reg_ptr->sts & 0x1)

• coresight_insert_barrier_packet(drvdata->buf);
  

• drvdata->reading = true;

+out:

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return ret;

+}

+static int tmc_etb_unprepare_prevboot(struct tmc_drvdata *drvdata) {

• unsigned long flags;
• spin_lock_irqsave(&drvdata->spinlock, flags);
• tmc_etb_free_prevboot_buf(drvdata);
• drvdata->reading = false;
• spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return 0;

+}

int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) { enum tmc_mode mode; @@ -675,6 +754,9 @@ int tmc_read_prepare_etb(struct tmc_drvdata

*drvdata)

...

	 drvdata->config_type != TMC_CONFIG_TYPE_ETF))
return -EINVAL;

• if (drvdata->mode == CS_MODE_READ_PREVBOOT)

• return tmc_etb_prepare_prevboot(drvdata);
  

Can you move this inside the spinlock below, and remove the spinlock from tmc_etb_prepare_prevboot()? Otherwise it looks a bit like it's a special case that is supposed to happen outside of the lock until you go and look inside the function, but then all the locking code is just repeated.

I think the same applies for the ETR one.

Ack.

...

spin_lock_irqsave(&drvdata->spinlock, flags);

if (drvdata->reading) { @@ -724,6 +806,9 @@ int tmc_read_unprepare_etb(struct tmc_drvdata

*drvdata)

...

	 drvdata->config_type != TMC_CONFIG_TYPE_ETF))
return -EINVAL;

• if (drvdata->mode == CS_MODE_READ_PREVBOOT)

• return tmc_etb_unprepare_prevboot(drvdata);
  

• spin_lock_irqsave(&drvdata->spinlock, flags);

  /* Re-enable the TMC if need be */

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index a99b3e9fb9ec..6e8d4069a3fa 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1159,7 +1159,12 @@ ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata, { s64 offset; ssize_t actual = len;

• struct etr_buf *etr_buf = drvdata->sysfs_buf;

• struct etr_buf *etr_buf;

• if (drvdata->mode == CS_MODE_READ_PREVBOOT)

• etr_buf = drvdata->prevboot_buf;
  

• else

• etr_buf = drvdata->sysfs_buf;
  
  

  if (pos + actual > etr_buf->len) actual = etr_buf->len - pos;

@@ -1843,6 +1848,147 @@ const struct coresight_ops tmc_etr_cs_ops = { .panic_ops = &tmc_etr_sync_ops, };

+static int tmc_etr_setup_prevboot_buf(struct tmc_drvdata *drvdata) {

• int rc = 0;
• u64 trace_addr;
• struct etr_buf *etr_buf;
• struct etr_flat_buf *resrv_buf;
• struct tmc_register_snapshot *reg_ptr;
• etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL);
• if (!etr_buf) {

• rc = -ENOMEM;
  

• goto out;
  

• }
• etr_buf->size = drvdata->size;
• resrv_buf = kzalloc(sizeof(*resrv_buf), GFP_KERNEL);
• if (!resrv_buf) {

• rc = -ENOMEM;
  

• goto rmem_err;
  

• }
• reg_ptr = drvdata->metadata.vaddr;
• trace_addr = reg_ptr->trc_addr | ((u64)reg_ptr->trc_addrhi << 32);
• resrv_buf->vaddr = memremap(trace_addr, reg_ptr->size * 4,

MEMREMAP_WC);

...

• if (IS_ERR(drvdata->buf)) {

• rc = -ENOMEM;
  

• goto map_err;
  

• }
• resrv_buf->size = etr_buf->size;
• resrv_buf->dev = &drvdata->csdev->dev;
• etr_buf->hwaddr = trace_addr;
• etr_buf->mode = ETR_MODE_RESRV;
• etr_buf->private = resrv_buf;
• etr_buf->ops = etr_buf_ops[ETR_MODE_RESRV];
• drvdata->prevboot_buf = etr_buf;
• return 0;

+map_err:

• kfree(resrv_buf);

+rmem_err:

• kfree(etr_buf);

+out:

• return rc;

+}

+static int tmc_etr_sync_prevboot_buf(struct tmc_drvdata *drvdata) {

• u32 status;
• u64 rrp, rwp, dba;
• struct tmc_register_snapshot *reg_ptr;
• struct etr_buf *etr_buf = drvdata->prevboot_buf;
• reg_ptr = drvdata->metadata.vaddr;
• rrp = reg_ptr->rrp | ((u64)reg_ptr->rrphi << 32);
• rwp = reg_ptr->rwp | ((u64)reg_ptr->rwphi << 32);
• dba = reg_ptr->dba | ((u64)reg_ptr->dbahi << 32);
• status = reg_ptr->sts;
• etr_buf->full = !!(status & TMC_STS_FULL);
• /* Sync the buffer pointers */
• etr_buf->offset = rrp - dba;
• if (etr_buf->full)

• etr_buf->len = etr_buf->size;
  

• else

• etr_buf->len = rwp - rrp;
  

• return 0;

+}

+static void tmc_etr_free_prevboot_buf(struct tmc_drvdata *drvdata) {

• void *buf = drvdata->prevboot_buf;
• if (!buf)

• return;
  

• memunmap(buf);
• drvdata->prevboot_buf = NULL;

+}

+static int tmc_etr_prepare_prevboot(struct tmc_drvdata *drvdata) {

• struct tmc_register_snapshot *reg_ptr;
• unsigned long flags;
• int ret = 0;
• spin_lock_irqsave(&drvdata->spinlock, flags);
• if (drvdata->reading) {

• ret = -EBUSY;
  

• goto out;
  

• }
• if (!drvdata->metadata.vaddr) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• reg_ptr = drvdata->metadata.vaddr;
• if (!reg_ptr->valid) {

• dev_err(&drvdata->csdev->dev,
  

• 	"Invalid metadata captured from previous boot\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }
• ret = tmc_etr_setup_prevboot_buf(drvdata);
• if (ret)

• goto out;
  

• tmc_etr_sync_prevboot_buf(drvdata);
• if (reg_ptr->sts & 0x1)

• coresight_insert_barrier_packet(drvdata->buf);
  

• drvdata->reading = true;

+out:

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return ret;

+}

There seems to be mainly duplicated code between tmc_etr_prepare_prevboot() and tmc_etb_prepare_prevboot(). Is there any way of factoring out the common stuff and including some specialised callback in the middle like:

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,

•  .prepare_prevboot = &tmc_etb_prepare_prevboot,
  

};

Okay, will do.

...

+static int tmc_etr_unprepare_prevboot(struct tmc_drvdata *drvdata) {

• unsigned long flags;
• spin_lock_irqsave(&drvdata->spinlock, flags);
• tmc_etr_free_prevboot_buf(drvdata);
• drvdata->reading = false;
• spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return 0;

+}

int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) { int ret = 0; @@ -1852,6 +1998,9 @@ int tmc_read_prepare_etr(struct tmc_drvdata

*drvdata)

...

if (WARN_ON_ONCE(drvdata->config_type !=

TMC_CONFIG_TYPE_ETR))

...

return -EINVAL;

• if (drvdata->mode == CS_MODE_READ_PREVBOOT)

• return tmc_etr_prepare_prevboot(drvdata);
  

• spin_lock_irqsave(&drvdata->spinlock, flags); if (drvdata->reading) { ret = -EBUSY;

@@ -1888,6 +2037,9 @@ int tmc_read_unprepare_etr(struct tmc_drvdata

*drvdata)

...

if (WARN_ON_ONCE(drvdata->config_type !=

TMC_CONFIG_TYPE_ETR))

...

return -EINVAL;

• if (drvdata->mode == CS_MODE_READ_PREVBOOT)

• return tmc_etr_unprepare_prevboot(drvdata);
  

• spin_lock_irqsave(&drvdata->spinlock, flags);

  /* RE-enable the TMC if need be */

diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 8ebf624b26d9..64d80d95e5eb 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -217,6 +217,7 @@ struct tmc_resrv_buf {

• @idr_mutex: Access serialisation for idr.
• @sysfs_buf: SYSFS buffer for ETR.
• @perf_buf: PERF buffer for ETR.

• • @prevboot_buf: Previous boot buffer for ETR
  • @resrv_buf: Reserved Memory for trace data buffer. Used by ETR/ETF.
  • @metadata: Reserved memory for metadata. Used by ETR/ETF.
  */

@@ -243,6 +244,7 @@ struct tmc_drvdata { struct mutex idr_mutex; struct etr_buf *sysfs_buf; struct etr_buf *perf_buf;

• struct etr_buf *prevboot_buf;

Why does ETR have this new member, but ETB uses the existing *buf for the previous buffer?

Since there was a separate etr buf for each mode, thought it was more inline with the current style of implementation. Basically the setting up of buffer is different for PREVBOOT mode. Ie. Initialization of sysf_buf involves, memory allocation, dma mapping etc while for the prevboot_buf, we just need to do the memremap.

Will add necessary comments for this.

Maybe a comment here or where it is used would help.

...

struct tmc_resrv_buf resrv_buf; struct tmc_resrv_buf metadata; };