Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC 07/11] coresight: sink: Add TRBE driver
To:     Anshuman Khandual <anshuman.khandual@arm.com>,
        linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org
Cc:     linux-kernel@vger.kernel.org, mathieu.poirier@linaro.org,
        mike.leach@linaro.org
References: <1605012309-24812-1-git-send-email-anshuman.khandual@arm.com>
 <1605012309-24812-8-git-send-email-anshuman.khandual@arm.com>
From:   Suzuki K Poulose <suzuki.poulose@arm.com>
Message-ID: <27d318eb-8539-698c-9c33-309dcacecb99@arm.com>
Date:   Thu, 12 Nov 2020 10:13:11 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.1
MIME-Version: 1.0
In-Reply-To: <1605012309-24812-8-git-send-email-anshuman.khandual@arm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/10/20 12:45 PM, Anshuman Khandual wrote:
> Trace Buffer Extension (TRBE) implements a trace buffer per CPU which is
> accessible via the system registers. The TRBE supports different addressing
> modes including CPU virtual address and buffer modes including the circular
> buffer mode. The TRBE buffer is addressed by a base pointer (TRBBASER_EL1),
> an write pointer (TRBPTR_EL1) and a limit pointer (TRBLIMITR_EL1). But the
> access to the trace buffer could be prohibited by a higher exception level
> (EL3 or EL2), indicated by TRBIDR_EL1.P. The TRBE can also generate a CPU
> private interrupt (PPI) on address translation errors and when the buffer
> is full. Overall implementation here is inspired from the Arm SPE driver.
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>   Documentation/trace/coresight/coresight-trbe.rst |  36 ++
>   arch/arm64/include/asm/sysreg.h                  |   2 +
>   drivers/hwtracing/coresight/Kconfig              |  11 +
>   drivers/hwtracing/coresight/Makefile             |   1 +
>   drivers/hwtracing/coresight/coresight-trbe.c     | 766 +++++++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-trbe.h     | 525 ++++++++++++++++
>   6 files changed, 1341 insertions(+)
>   create mode 100644 Documentation/trace/coresight/coresight-trbe.rst
>   create mode 100644 drivers/hwtracing/coresight/coresight-trbe.c
>   create mode 100644 drivers/hwtracing/coresight/coresight-trbe.h
> 
> diff --git a/Documentation/trace/coresight/coresight-trbe.rst b/Documentation/trace/coresight/coresight-trbe.rst
> new file mode 100644
> index 0000000..4320a8b
> --- /dev/null
> +++ b/Documentation/trace/coresight/coresight-trbe.rst
> @@ -0,0 +1,36 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==============================
> +Trace Buffer Extension (TRBE).
> +==============================
> +
> +    :Author:   Anshuman Khandual <anshuman.khandual@arm.com>
> +    :Date:     November 2020
> +
> +Hardware Description
> +--------------------
> +
> +Trace Buffer Extension (TRBE) is a percpu hardware which captures in system
> +memory, CPU traces generated from a corresponding percpu tracing unit. This
> +gets plugged in as a coresight sink device because the corresponding trace
> +genarators (ETE), are plugged in as source device.
> +
> +Sysfs files and directories
> +---------------------------
> +
> +The TRBE devices appear on the existing coresight bus alongside the other
> +coresight devices::
> +
> +	>$ ls /sys/bus/coresight/devices
> +	trbe0  trbe1  trbe2 trbe3
> +
> +The ``trbe<N>`` named TRBEs are associated with a CPU.::
> +
> +	>$ ls /sys/bus/coresight/devices/trbe0/
> +	irq align dbm
> +
> +*Key file items are:-*
> +   * ``irq``: TRBE maintenance interrupt number
> +   * ``align``: TRBE write pointer alignment
> +   * ``dbm``: TRBE updates memory with access and dirty flags
> +
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 14cb156..61136f6 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -97,6 +97,7 @@
>   #define SET_PSTATE_UAO(x)		__emit_inst(0xd500401f | PSTATE_UAO | ((!!x) << PSTATE_Imm_shift))
>   #define SET_PSTATE_SSBS(x)		__emit_inst(0xd500401f | PSTATE_SSBS | ((!!x) << PSTATE_Imm_shift))
>   #define SET_PSTATE_TCO(x)		__emit_inst(0xd500401f | PSTATE_TCO | ((!!x) << PSTATE_Imm_shift))
> +#define TSB_CSYNC			__emit_inst(0xd503225f)
>   
>   #define __SYS_BARRIER_INSN(CRm, op2, Rt) \
>   	__emit_inst(0xd5000000 | sys_insn(0, 3, 3, (CRm), (op2)) | ((Rt) & 0x1f))
> @@ -865,6 +866,7 @@
>   #define ID_AA64MMFR2_CNP_SHIFT		0
>   
>   /* id_aa64dfr0 */
> +#define ID_AA64DFR0_TRBE_SHIFT		44
>   #define ID_AA64DFR0_TRACE_FILT_SHIFT	40
>   #define ID_AA64DFR0_DOUBLELOCK_SHIFT	36
>   #define ID_AA64DFR0_PMSVER_SHIFT	32
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index c119824..0f5e101 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -156,6 +156,17 @@ config CORESIGHT_CTI
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called coresight-cti.
>   
> +config CORESIGHT_TRBE
> +	bool "Trace Buffer Extension (TRBE) driver"
> +	depends on ARM64
> +	help
> +	  This driver provides support for percpu Trace Buffer Extension (TRBE).
> +	  TRBE always needs to be used along with it's corresponding percpu ETE
> +	  component. ETE generates trace data which is then captured with TRBE.
> +	  Unlike traditional sink devices, TRBE is a CPU feature accessible via
> +	  system registers. But it's explicit dependency with trace unit (ETE)
> +	  requires it to be plugged in as a coresight sink device.
> +
>   config CORESIGHT_CTI_INTEGRATION_REGS
>   	bool "Access CTI CoreSight Integration Registers"
>   	depends on CORESIGHT_CTI
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index f20e357..d608165 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -21,5 +21,6 @@ obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
>   obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
>   obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o
>   obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o
> +obj-$(CONFIG_CORESIGHT_TRBE) += coresight-trbe.o
>   coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
>   		   coresight-cti-sysfs.o
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> new file mode 100644
> index 0000000..48a8ec3
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -0,0 +1,766 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This driver enables Trace Buffer Extension (TRBE) as a per-cpu coresight
> + * sink device could then pair with an appropriate per-cpu coresight source
> + * device (ETE) thus generating required trace data. Trace can be enabled
> + * via the perf framework.
> + *
> + * Copyright (C) 2020 ARM Ltd.
> + *
> + * Author: Anshuman Khandual <anshuman.khandual@arm.com>
> + */
> +#define DRVNAME "arm_trbe"
> +
> +#define pr_fmt(fmt) DRVNAME ": " fmt
> +
> +#include "coresight-trbe.h"
> +
> +#define PERF_IDX2OFF(idx, buf) ((idx) % ((buf)->nr_pages << PAGE_SHIFT))
> +
> +#define ETE_IGNORE_PACKET 0x70

Add a comment here, on what this means to the decoder.

> +
> +static const char trbe_name[] = "trbe";

Why not

#define DEVNAME	"trbe"


> +
> +enum trbe_fault_action {
> +	TRBE_FAULT_ACT_WRAP,
> +	TRBE_FAULT_ACT_SPURIOUS,
> +	TRBE_FAULT_ACT_FATAL,
> +};
> +
> +struct trbe_perf {

Please rename this to trbe_buf. This will be used for sysfs mode as well.

> +	unsigned long trbe_base;
> +	unsigned long trbe_limit;
> +	unsigned long trbe_write;
> +	pid_t pid;

Why do we need this ? This seems unused and moreover, there cannot
be multiple tracers into TRBE. So, we don't need to share the sink
unlike the traditional ones.

> +	int nr_pages;
> +	void **pages;
> +	bool snapshot;
> +	struct trbe_cpudata *cpudata;
> +};
> +
> +struct trbe_cpudata {
> +	struct coresight_device	*csdev;
> +	bool trbe_dbm;

Why do we need this ?

> +	u64 trbe_align;
> +	int cpu;
> +	enum cs_mode mode;
> +	struct trbe_perf *perf;
> +	struct trbe_drvdata *drvdata;
> +};
> +
> +struct trbe_drvdata {
> +	struct trbe_cpudata __percpu *cpudata;
> +	struct perf_output_handle __percpu *handle;

Shouldn't this be :

	struct perf_output_handle __percpu **handle ?

as we get a handle from the etm-perf and is not controlled by
the TRBE ?

> +	struct hlist_node hotplug_node;
> +	int irq;
> +	cpumask_t supported_cpus;
> +	enum cpuhp_state trbe_online;
> +	struct platform_device *pdev;
> +	struct clk *atclk;

We don't have any clocks for the TRBE instance. Please remove.

> +};
> +
> +static int trbe_alloc_node(struct perf_event *event)
> +{
> +	if (event->cpu == -1)
> +		return NUMA_NO_NODE;
> +	return cpu_to_node(event->cpu);
> +}
> +
> +static void trbe_disable_and_drain_local(void)
> +{
> +	write_sysreg_s(0, SYS_TRBLIMITR_EL1);
> +	isb();
> +	dsb(nsh);
> +	asm(TSB_CSYNC);
> +}
> +
> +static void trbe_reset_local(void)
> +{
> +	trbe_disable_and_drain_local();
> +	write_sysreg_s(0, SYS_TRBPTR_EL1);
> +	isb();
> +
> +	write_sysreg_s(0, SYS_TRBBASER_EL1);
> +	isb();
> +
> +	write_sysreg_s(0, SYS_TRBSR_EL1);
> +	isb();
> +}
> +
> +static void trbe_pad_buf(struct perf_output_handle *handle, int len)
> +{
> +	struct trbe_perf *perf = etm_perf_sink_config(handle);
> +	u64 head = PERF_IDX2OFF(handle->head, perf);
> +
> +	memset((void *) perf->trbe_base + head, ETE_IGNORE_PACKET, len);
> +	if (!perf->snapshot)
> +		perf_aux_output_skip(handle, len);
> +}
> +
> +static unsigned long trbe_snapshot_offset(struct perf_output_handle *handle)
> +{
> +	struct trbe_perf *perf = etm_perf_sink_config(handle);
> +	u64 head = PERF_IDX2OFF(handle->head, perf);
> +	u64 limit = perf->nr_pages * PAGE_SIZE;
> +

So we are using half of the buffer for snapshot mode to avoid a case where the
analyzer is unable to decode the trace in case of an overflow.

> +	if (head < limit >> 1)
> +		limit >>= 1;

Also this needs to be thought out. We may not need this restriction. The trace decoder
will be able to walk forward and then find a synchronization packet and then continue
the tracing from there. So, we could use the entire buffer for TRBE.


> +
> +	return limit;
> +}
> +
> +static unsigned long trbe_normal_offset(struct perf_output_handle *handle)
> +{
> +	struct trbe_perf *perf = etm_perf_sink_config(handle);
> +	struct trbe_cpudata *cpudata = perf->cpudata;
> +	const u64 bufsize = perf->nr_pages * PAGE_SIZE;
> +	u64 limit = bufsize;
> +	u64 head, tail, wakeup;
> +

Commentary please.

> +	head = PERF_IDX2OFF(handle->head, perf);
> +	if (!IS_ALIGNED(head, cpudata->trbe_align)) {
> +		unsigned long delta = roundup(head, cpudata->trbe_align) - head;
> +
> +		delta = min(delta, handle->size);
> +		trbe_pad_buf(handle, delta);
> +		head = PERF_IDX2OFF(handle->head, perf);
> +	}
> +
> +	if (!handle->size) {
> +		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +		return 0;
> +	}
> +
> +	tail = PERF_IDX2OFF(handle->head + handle->size, perf);
> +	wakeup = PERF_IDX2OFF(handle->wakeup, perf);
> +

> +	if (head < tail)

  comment

> +		limit = round_down(tail, PAGE_SIZE);
> +
> +	if (handle->wakeup < (handle->head + handle->size) && head <= wakeup)
> +		limit = min(limit, round_up(wakeup, PAGE_SIZE));

comment. Also do we need an alignement to PAGE_SIZE ?

> +
> +	if (limit > head)
> +		return limit;
> +
> +	trbe_pad_buf(handle, handle->size);
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +	return 0;
> +}
> +
> +static unsigned long get_trbe_limit(struct perf_output_handle *handle)
> +{
> +	struct trbe_perf *perf = etm_perf_sink_config(handle);
> +	unsigned long offset;
> +
> +	if (perf->snapshot)
> +		offset = trbe_snapshot_offset(handle);
> +	else
> +		offset = trbe_normal_offset(handle);
> +	return perf->trbe_base + offset;
> +}
> +
> +static void trbe_enable_hw(struct trbe_perf *perf)
> +{
> +	WARN_ON(perf->trbe_write < perf->trbe_base);
> +	WARN_ON(perf->trbe_write >= perf->trbe_limit);
> +	set_trbe_disabled();
> +	clr_trbe_irq();
> +	clr_trbe_wrap();
> +	clr_trbe_abort();
> +	clr_trbe_ec();
> +	clr_trbe_bsc();
> +	clr_trbe_fsc();

Please merge all of these field updates to single register update
unless mandated by the architecture.

> +	set_trbe_virtual_mode();
> +	set_trbe_fill_mode(TRBE_FILL_STOP);
> +	set_trbe_trig_mode(TRBE_TRIGGER_IGNORE);

Same here ^^

> +	isb();
> +	set_trbe_base_pointer(perf->trbe_base);
> +	set_trbe_limit_pointer(perf->trbe_limit);
> +	set_trbe_write_pointer(perf->trbe_write);
> +	isb();
> +	dsb(ishst);
> +	flush_tlb_all();

Why is this needed ?

> +	set_trbe_running();
> +	set_trbe_enabled();
> +	asm(TSB_CSYNC);
> +}
> +
> +static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
> +				   struct perf_event *event, void **pages,
> +				   int nr_pages, bool snapshot)
> +{
> +	struct trbe_perf *perf;
> +	struct page **pglist;
> +	int i;
> +
> +	if ((nr_pages < 2) || (snapshot && (nr_pages & 1)))

We may be able to remove the restriction on snapshot mode, see my comment
above.

> +		return NULL;
> +
> +	perf = kzalloc_node(sizeof(*perf), GFP_KERNEL, trbe_alloc_node(event));
> +	if (IS_ERR(perf))
> +		return ERR_PTR(-ENOMEM);
> +
> +	pglist = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
> +	if (IS_ERR(pglist)) {
> +		kfree(perf);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	for (i = 0; i < nr_pages; i++)
> +		pglist[i] = virt_to_page(pages[i]);
> +
> +	perf->trbe_base = (unsigned long) vmap(pglist, nr_pages, VM_MAP, PAGE_KERNEL);
> +	if (IS_ERR((void *) perf->trbe_base)) {
> +		kfree(pglist);
> +		kfree(perf);
> +		return ERR_PTR(perf->trbe_base);
> +	}
> +	perf->trbe_limit = perf->trbe_base + nr_pages * PAGE_SIZE;
> +	perf->trbe_write = perf->trbe_base;
> +	perf->pid = task_pid_nr(event->owner);
> +	perf->snapshot = snapshot;
> +	perf->nr_pages = nr_pages;
> +	perf->pages = pages;
> +	kfree(pglist);
> +	return perf;
> +}
> +
> +void arm_trbe_free_buffer(void *config)
> +{
> +	struct trbe_perf *perf = config;
> +
> +	vunmap((void *) perf->trbe_base);
> +	kfree(perf);
> +}
> +
> +static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
> +					    struct perf_output_handle *handle,
> +					    void *config)
> +{
> +	struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> +	struct trbe_perf *perf = config;
> +	unsigned long size, offset;
> +
> +	WARN_ON(perf->cpudata != cpudata);
> +	WARN_ON(cpudata->cpu != smp_processor_id());
> +	WARN_ON(cpudata->mode != CS_MODE_PERF);
> +	WARN_ON(cpudata->drvdata != drvdata);
> +
> +	offset = get_trbe_write_pointer() - get_trbe_base_pointer();
> +	size = offset - PERF_IDX2OFF(handle->head, perf);
> +	if (perf->snapshot)
> +		handle->head += size;
> +	return size;
> +}
> +
> +static int arm_trbe_enable(struct coresight_device *csdev, u32 mode, void *data)
> +{
> +	struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> +	struct perf_output_handle *handle = data;
> +	struct trbe_perf *perf = etm_perf_sink_config(handle);
> +
> +	WARN_ON(cpudata->cpu != smp_processor_id());
> +	WARN_ON(mode != CS_MODE_PERF);

Why WARN_ON ? Simply return -EINVAL ? Also you need a check to make sure
the mode is DISABLED (when you get to sysfs mode).

> +	WARN_ON(cpudata->drvdata != drvdata);
> +
> +	*this_cpu_ptr(drvdata->handle) = *handle;

That is wrong. Storing a local copy of a global perf generic structure
is calling for trouble, assuming that the global structure doesn't change
beneath us. Please store handle ptr.

> +	cpudata->perf = perf;
> +	cpudata->mode = mode;
> +	perf->cpudata = cpudata;
> +	perf->trbe_write = perf->trbe_base + PERF_IDX2OFF(handle->head, perf);
> +	perf->trbe_limit = get_trbe_limit(handle);
> +	if (perf->trbe_limit == perf->trbe_base) {
> +		trbe_disable_and_drain_local();
> +		return 0;
> +	}
> +	trbe_enable_hw(perf);
> +	return 0;
> +}
> +
> +static int arm_trbe_disable(struct coresight_device *csdev)
> +{
> +	struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> +	struct trbe_perf *perf = cpudata->perf;
> +
> +	WARN_ON(perf->cpudata != cpudata);
> +	WARN_ON(cpudata->cpu != smp_processor_id());
> +	WARN_ON(cpudata->mode != CS_MODE_PERF);
> +	WARN_ON(cpudata->drvdata != drvdata);
> +
> +	trbe_disable_and_drain_local();
> +	perf->cpudata = NULL;
> +	cpudata->perf = NULL;
> +	cpudata->mode = CS_MODE_DISABLED;
> +	return 0;
> +}
> +
> +static void trbe_handle_fatal(struct perf_output_handle *handle)
> +{
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +	perf_aux_output_end(handle, 0);
> +	trbe_disable_and_drain_local();
> +}
> +
> +static void trbe_handle_spurious(struct perf_output_handle *handle)
> +{
> +	struct trbe_perf *perf = etm_perf_sink_config(handle);
> +
> +	perf->trbe_write = perf->trbe_base + PERF_IDX2OFF(handle->head, perf);
> +	perf->trbe_limit = get_trbe_limit(handle);
> +	if (perf->trbe_limit == perf->trbe_base) {
> +		trbe_disable_and_drain_local();
> +		return;
> +	}
> +	trbe_enable_hw(perf);
> +}
> +
> +static void trbe_handle_overflow(struct perf_output_handle *handle)
> +{
> +	struct perf_event *event = handle->event;
> +	struct trbe_perf *perf = etm_perf_sink_config(handle);
> +	unsigned long offset, size;
> +	struct etm_event_data *event_data;
> +
> +	offset = get_trbe_limit_pointer() - get_trbe_base_pointer();
> +	size = offset - PERF_IDX2OFF(handle->head, perf);
> +	if (perf->snapshot)
> +		handle->head = offset;

Is this correct ? Or was this supposed to mean :
		handle->head += offset;


> +	perf_aux_output_end(handle, size);
> +
> +	event_data = perf_aux_output_begin(handle, event);
> +	if (!event_data) {
> +		event->hw.state |= PERF_HES_STOPPED;
> +		trbe_disable_and_drain_local();
> +		return;
> +	}
> +	perf->trbe_write = perf->trbe_base;
> +	perf->trbe_limit = get_trbe_limit(handle);
> +	if (perf->trbe_limit == perf->trbe_base) {
> +		trbe_disable_and_drain_local();
> +		return;
> +	}
> +	*this_cpu_ptr(perf->cpudata->drvdata->handle) = *handle;
> +	trbe_enable_hw(perf);
> +}
> +
> +static bool is_perf_trbe(struct perf_output_handle *handle)
> +{
> +	struct trbe_perf *perf = etm_perf_sink_config(handle);
> +	struct trbe_cpudata *cpudata = perf->cpudata;
> +	struct trbe_drvdata *drvdata = cpudata->drvdata;

Can you trust the cpudata ptr here as we are still verifying
if this was legitimate ?

> +	int cpu = smp_processor_id();
> +
> +	WARN_ON(perf->trbe_base != get_trbe_base_pointer());
> +	WARN_ON(perf->trbe_limit != get_trbe_limit_pointer());
> +
> +	if (cpudata->mode != CS_MODE_PERF)
> +		return false;
> +
> +	if (cpudata->cpu != cpu)
> +		return false;
> +
> +	if (!cpumask_test_cpu(cpu, &drvdata->supported_cpus))
> +		return false;
> +
> +	return true;
> +}
> +
> +static enum trbe_fault_action trbe_get_fault_act(struct perf_output_handle *handle)
> +{
> +	enum trbe_ec ec = get_trbe_ec();
> +	enum trbe_bsc bsc = get_trbe_bsc();
> +
> +	WARN_ON(is_trbe_running());
> +	asm(TSB_CSYNC);
> +	dsb(nsh);
> +	isb();
> +
> +	if (is_trbe_trg() || is_trbe_abort())
> +		return TRBE_FAULT_ACT_FATAL;
> +
> +	if ((ec == TRBE_EC_STAGE1_ABORT) || (ec == TRBE_EC_STAGE2_ABORT))
> +		return TRBE_FAULT_ACT_FATAL;
> +
> +	if (is_trbe_wrap() && (ec == TRBE_EC_OTHERS) && (bsc == TRBE_BSC_FILLED)) {
> +		if (get_trbe_write_pointer() == get_trbe_base_pointer())
> +			return TRBE_FAULT_ACT_WRAP;
> +	}
> +	return TRBE_FAULT_ACT_SPURIOUS;
> +}
> +
> +static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
> +{
> +	struct perf_output_handle *handle = dev;
> +	enum trbe_fault_action act;
> +
> +	WARN_ON(!is_trbe_irq());
> +	clr_trbe_irq();
> +
> +	if (!perf_get_aux(handle))
> +		return IRQ_NONE;
> +
> +	if (!is_perf_trbe(handle))
> +		return IRQ_NONE;
> +
> +	irq_work_run();
> +
> +	act = trbe_get_fault_act(handle);
> +	switch (act) {
> +	case TRBE_FAULT_ACT_WRAP:
> +		trbe_handle_overflow(handle);
> +		break;
> +	case TRBE_FAULT_ACT_SPURIOUS:
> +		trbe_handle_spurious(handle);
> +		break;
> +	case TRBE_FAULT_ACT_FATAL:
> +		trbe_handle_fatal(handle);
> +		break;
> +	}
> +	return IRQ_HANDLED;
> +}
> +


> +static void arm_trbe_probe_coresight_cpu(void *info)
> +{
> +	struct trbe_cpudata *cpudata = info;
> +	struct device *dev = &cpudata->drvdata->pdev->dev;
> +	struct coresight_desc desc = { 0 };
> +
> +	if (WARN_ON(!cpudata))
> +		goto cpu_clear;
> +
> +	if (!is_trbe_available()) {
> +		pr_err("TRBE is not implemented on cpu %d\n", cpudata->cpu);
> +		goto cpu_clear;
> +	}
> +
> +	if (!is_trbe_programmable()) {
> +		pr_err("TRBE is owned in higher exception level on cpu %d\n", cpudata->cpu);
> +		goto cpu_clear;
> +	}
> +	desc.name = devm_kasprintf(dev, GFP_KERNEL, "%s%d", trbe_name, smp_processor_id());
> +	if (IS_ERR(desc.name))
> +		goto cpu_clear;
> +
> +	desc.type = CORESIGHT_DEV_TYPE_SINK;
> +	desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM;

May be should add a new subtype to make this higher priority than the normal ETR.
Something like :

	CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM

> +	desc.ops = &arm_trbe_cs_ops;
> +	desc.pdata = dev_get_platdata(dev);
> +	desc.groups = arm_trbe_groups;
> +	desc.dev = dev;
> +	cpudata->csdev = coresight_register(&desc);
> +	if (IS_ERR(cpudata->csdev))
> +		goto cpu_clear;
> +
> +	dev_set_drvdata(&cpudata->csdev->dev, cpudata);
> +	cpudata->trbe_dbm = get_trbe_flag_update();
> +	cpudata->trbe_align = 1ULL << get_trbe_address_align();
> +	if (cpudata->trbe_align > SZ_2K) {
> +		pr_err("Unsupported alignment on cpu %d\n", cpudata->cpu);
> +		goto cpu_clear;
> +	}
> +	return;
> +cpu_clear:
> +	cpumask_clear_cpu(cpudata->cpu, &cpudata->drvdata->supported_cpus);
> +}
> +
> +static int arm_trbe_probe_coresight(struct trbe_drvdata *drvdata)
> +{
> +	struct trbe_cpudata *cpudata;
> +	int cpu;
> +
> +	drvdata->cpudata = alloc_percpu(typeof(*drvdata->cpudata));
> +	if (IS_ERR(drvdata->cpudata))
> +		return PTR_ERR(drvdata->cpudata);
> +
> +	for_each_cpu(cpu, &drvdata->supported_cpus) {
> +		cpudata = per_cpu_ptr(drvdata->cpudata, cpu);
> +		cpudata->cpu = cpu;
> +		cpudata->drvdata = drvdata;
> +		smp_call_function_single(cpu, arm_trbe_probe_coresight_cpu, cpudata, 1);

We could batch it and run it on all CPUs at the same time ? Also it would be better to
leave the per_cpu area filled by the CPU itself, to avoid racing.


> +	}
> +	return 0;
> +}
> +
> +static void arm_trbe_remove_coresight_cpu(void *info)
> +{
> +	struct trbe_drvdata *drvdata = info;
> +
> +	disable_percpu_irq(drvdata->irq);
> +}
> +
> +static int arm_trbe_remove_coresight(struct trbe_drvdata *drvdata)
> +{
> +	struct trbe_cpudata *cpudata;
> +	int cpu;
> +
> +	for_each_cpu(cpu, &drvdata->supported_cpus) {
> +		smp_call_function_single(cpu, arm_trbe_remove_coresight_cpu, drvdata, 1);
> +		cpudata = per_cpu_ptr(drvdata->cpudata, cpu);
> +		if (cpudata->csdev) {
> +			coresight_unregister(cpudata->csdev);
> +			cpudata->drvdata = NULL;
> +			cpudata->csdev = NULL;
> +		}

Please leave this to the CPU to do the part.

> +	}
> +	free_percpu(drvdata->cpudata);
> +	return 0;
> +}
> +
> +static int arm_trbe_cpu_startup(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct trbe_drvdata *drvdata = hlist_entry_safe(node, struct trbe_drvdata, hotplug_node);
> +	struct trbe_cpudata *cpudata;
> +
> +	if (cpumask_test_cpu(cpu, &drvdata->supported_cpus)) {
> +		cpudata = per_cpu_ptr(drvdata->cpudata, cpu);
> +		if (!cpudata->csdev) {
> +			cpudata->drvdata = drvdata;
> +			smp_call_function_single(cpu, arm_trbe_probe_coresight_cpu, cpudata, 1);

Why do we need smp_call here ? We are already on the CPU.

> +		}
> +		trbe_reset_local();
> +		enable_percpu_irq(drvdata->irq, IRQ_TYPE_NONE);
> +	}
> +	return 0;
> +}
> +
> +static int arm_trbe_cpu_teardown(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct trbe_drvdata *drvdata = hlist_entry_safe(node, struct trbe_drvdata, hotplug_node);
> +	struct trbe_cpudata *cpudata;
> +
> +	if (cpumask_test_cpu(cpu, &drvdata->supported_cpus)) {
> +		cpudata = per_cpu_ptr(drvdata->cpudata, cpu);
> +		if (cpudata->csdev) {
> +			coresight_unregister(cpudata->csdev);
> +			cpudata->drvdata = NULL;
> +			cpudata->csdev = NULL;
> +		}
> +		disable_percpu_irq(drvdata->irq);
> +		trbe_reset_local();
> +	}
> +	return 0;
> +}
> +
> +static int arm_trbe_probe_cpuhp(struct trbe_drvdata *drvdata)
> +{
> +	enum cpuhp_state trbe_online;
> +
> +	trbe_online = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, DRVNAME,
> +					arm_trbe_cpu_startup, arm_trbe_cpu_teardown);
> +	if (trbe_online < 0)
> +		return -EINVAL;
> +
> +	if (cpuhp_state_add_instance(trbe_online, &drvdata->hotplug_node))
> +		return -EINVAL;
> +
> +	drvdata->trbe_online = trbe_online;
> +	return 0;
> +}
> +
> +static void arm_trbe_remove_cpuhp(struct trbe_drvdata *drvdata)
> +{
> +	cpuhp_remove_multi_state(drvdata->trbe_online);
> +}
> +
> +static int arm_trbe_probe_irq(struct platform_device *pdev,
> +			      struct trbe_drvdata *drvdata)
> +{
> +	drvdata->irq = platform_get_irq(pdev, 0);
> +	if (!drvdata->irq) {
> +		pr_err("IRQ not found for the platform device\n");
> +		return -ENXIO;
> +	}
> +
> +	if (!irq_is_percpu(drvdata->irq)) {
> +		pr_err("IRQ is not a PPI\n");
> +		return -EINVAL;
> +	}
> +
> +	if (irq_get_percpu_devid_partition(drvdata->irq, &drvdata->supported_cpus))
> +		return -EINVAL;
> +
> +	drvdata->handle = alloc_percpu(typeof(*drvdata->handle));
> +	if (!drvdata->handle)
> +		return -ENOMEM;
> +
> +	if (request_percpu_irq(drvdata->irq, arm_trbe_irq_handler, DRVNAME, drvdata->handle)) {
> +		free_percpu(drvdata->handle);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static void arm_trbe_remove_irq(struct trbe_drvdata *drvdata)
> +{
> +	free_percpu_irq(drvdata->irq, drvdata->handle);
> +	free_percpu(drvdata->handle);
> +}
> +
> +static int arm_trbe_device_probe(struct platform_device *pdev)
> +{
> +	struct coresight_platform_data *pdata;
> +	struct trbe_drvdata *drvdata;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (IS_ERR(drvdata))
> +		return -ENOMEM;
> +
> +	pdata = coresight_get_platform_data(dev);
> +	if (IS_ERR(pdata)) {
> +		kfree(drvdata);
> +		return -ENOMEM;
> +	}


> +
> +	drvdata->atclk = devm_clk_get(dev, "atclk");
> +	if (!IS_ERR(drvdata->atclk)) {
> +		ret = clk_prepare_enable(drvdata->atclk);
> +		if (ret)
> +			return ret;
> +	}

Please drop the clocks, we don't have any

> +	dev_set_drvdata(dev, drvdata);
> +	dev->platform_data = pdata;
> +	drvdata->pdev = pdev;
> +	ret = arm_trbe_probe_irq(pdev, drvdata);
> +	if (ret)
> +		goto irq_failed;
> +
> +	ret = arm_trbe_probe_coresight(drvdata);
> +	if (ret)
> +		goto probe_failed;
> +
> +	ret = arm_trbe_probe_cpuhp(drvdata);
> +	if (ret)
> +		goto cpuhp_failed;
> +
> +	return 0;
> +cpuhp_failed:
> +	arm_trbe_remove_coresight(drvdata);
> +probe_failed:
> +	arm_trbe_remove_irq(drvdata);
> +irq_failed:
> +	kfree(pdata);
> +	kfree(drvdata);
> +	return ret;
> +}
> +
> +static int arm_trbe_device_remove(struct platform_device *pdev)
> +{
> +	struct coresight_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct trbe_drvdata *drvdata = platform_get_drvdata(pdev);
> +
> +	arm_trbe_remove_coresight(drvdata);
> +	arm_trbe_remove_cpuhp(drvdata);
> +	arm_trbe_remove_irq(drvdata);
> +	kfree(pdata);
> +	kfree(drvdata);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int arm_trbe_runtime_suspend(struct device *dev)
> +{
> +	struct trbe_drvdata *drvdata = dev_get_drvdata(dev);
> +
> +	if (drvdata && !IS_ERR(drvdata->atclk))
> +		clk_disable_unprepare(drvdata->atclk);
> +

Remove. We may need to save/restore the TRBE ptrs, depending on the
TRBE.

> +	return 0;
> +}
> +
> +static int arm_trbe_runtime_resume(struct device *dev)
> +{
> +	struct trbe_drvdata *drvdata = dev_get_drvdata(dev);
> +
> +	if (drvdata && !IS_ERR(drvdata->atclk))
> +		clk_prepare_enable(drvdata->atclk);

Remove. See above.

> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops arm_trbe_dev_pm_ops = {
> +	SET_RUNTIME_PM_OPS(arm_trbe_runtime_suspend, arm_trbe_runtime_resume, NULL)
> +};
> +
> +static const struct of_device_id arm_trbe_of_match[] = {
> +	{ .compatible = "arm,arm-trbe",	.data = (void *)1 },
> +	{},
> +};

I think it is better to call this, we have too many acronyms ;-)

	"arm,trace-buffer-extension"

> +MODULE_DEVICE_TABLE(of, arm_trbe_of_match);

> +
> +static const struct platform_device_id arm_trbe_match[] = {
> +	{ "arm,trbe", 0},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, arm_trbe_match);

Please remove. The ACPI part can be added when we get to it.

> +
> +static struct platform_driver arm_trbe_driver = {
> +	.id_table = arm_trbe_match,
> +	.driver	= {
> +		.name = DRVNAME,
> +		.of_match_table = of_match_ptr(arm_trbe_of_match),
> +		.pm = &arm_trbe_dev_pm_ops,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe	= arm_trbe_device_probe,
> +	.remove	= arm_trbe_device_remove,
> +};
> +builtin_platform_driver(arm_trbe_driver)

Please make this modular.


> diff --git a/drivers/hwtracing/coresight/coresight-trbe.h b/drivers/hwtracing/coresight/coresight-trbe.h
> new file mode 100644
> index 0000000..82ffbfc
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-trbe.h
> @@ -0,0 +1,525 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This contains all required hardware related helper functions for
> + * Trace Buffer Extension (TRBE) driver in the coresight framework.
> + *
> + * Copyright (C) 2020 ARM Ltd.
> + *
> + * Author: Anshuman Khandual <anshuman.khandual@arm.com>
> + */
> +#include <linux/coresight.h>
> +#include <linux/device.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/smp.h>
> +
> +#include "coresight-etm-perf.h"
> +
> +static inline bool is_trbe_available(void)
> +{
> +	u64 aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> +	int trbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_TRBE_SHIFT);
> +
> +	return trbe >= 0b0001;
> +}
> +
> +static inline bool is_ete_available(void)
> +{
> +	u64 aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> +	int tracever = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_TRACEVER_SHIFT);
> +
> +	return (tracever != 0b0000);

Why is this needed ?

> +}
> +
> +static inline bool is_trbe_enabled(void)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	return trblimitr & TRBLIMITR_ENABLE;
> +}
> +
> +enum trbe_ec {
> +	TRBE_EC_OTHERS		= 0,
> +	TRBE_EC_STAGE1_ABORT	= 36,
> +	TRBE_EC_STAGE2_ABORT	= 37,
> +};
> +
> +static const char *const trbe_ec_str[] = {
> +	[TRBE_EC_OTHERS]	= "Maintenance exception",
> +	[TRBE_EC_STAGE1_ABORT]	= "Stage-1 exception",
> +	[TRBE_EC_STAGE2_ABORT]	= "Stage-2 exception",
> +};
> +

Please remove the defintions that are not used by the driver.

> +static inline enum trbe_ec get_trbe_ec(void)
> +{
> +	u64 trbsr = read_sysreg_s(SYS_TRBSR_EL1);
> +
> +	return (trbsr >> TRBSR_EC_SHIFT) & TRBSR_EC_MASK;
> +}
> +
> +static inline void clr_trbe_ec(void)
> +{
> +	u64 trbsr = read_sysreg_s(SYS_TRBSR_EL1);
> +
> +	trbsr &= ~(TRBSR_EC_MASK << TRBSR_EC_SHIFT);
> +	write_sysreg_s(trbsr, SYS_TRBSR_EL1);
> +}
> +
> +enum trbe_bsc {
> +	TRBE_BSC_NOT_STOPPED	= 0,
> +	TRBE_BSC_FILLED		= 1,
> +	TRBE_BSC_TRIGGERED	= 2,
> +};
> +
> +static const char *const trbe_bsc_str[] = {
> +	[TRBE_BSC_NOT_STOPPED]	= "TRBE collection not stopped",
> +	[TRBE_BSC_FILLED]	= "TRBE filled",
> +	[TRBE_BSC_TRIGGERED]	= "TRBE triggered",
> +};
> +
> +static inline enum trbe_bsc get_trbe_bsc(void)
> +{
> +	u64 trbsr = read_sysreg_s(SYS_TRBSR_EL1);
> +
> +	return (trbsr >> TRBSR_BSC_SHIFT) & TRBSR_BSC_MASK;
> +}
> +
> +static inline void clr_trbe_bsc(void)
> +{
> +	u64 trbsr = read_sysreg_s(SYS_TRBSR_EL1);
> +
> +	trbsr &= ~(TRBSR_BSC_MASK << TRBSR_BSC_SHIFT);
> +	write_sysreg_s(trbsr, SYS_TRBSR_EL1);
> +}
> +
> +enum trbe_fsc {
> +	TRBE_FSC_ASF_LEVEL0	= 0,
> +	TRBE_FSC_ASF_LEVEL1	= 1,
> +	TRBE_FSC_ASF_LEVEL2	= 2,
> +	TRBE_FSC_ASF_LEVEL3	= 3,
> +	TRBE_FSC_TF_LEVEL0	= 4,
> +	TRBE_FSC_TF_LEVEL1	= 5,
> +	TRBE_FSC_TF_LEVEL2	= 6,
> +	TRBE_FSC_TF_LEVEL3	= 7,
> +	TRBE_FSC_AFF_LEVEL0	= 8,
> +	TRBE_FSC_AFF_LEVEL1	= 9,
> +	TRBE_FSC_AFF_LEVEL2	= 10,
> +	TRBE_FSC_AFF_LEVEL3	= 11,
> +	TRBE_FSC_PF_LEVEL0	= 12,
> +	TRBE_FSC_PF_LEVEL1	= 13,
> +	TRBE_FSC_PF_LEVEL2	= 14,
> +	TRBE_FSC_PF_LEVEL3	= 15,
> +	TRBE_FSC_SEA_WRITE	= 16,
> +	TRBE_FSC_ASEA_WRITE	= 17,
> +	TRBE_FSC_SEA_LEVEL0	= 20,
> +	TRBE_FSC_SEA_LEVEL1	= 21,
> +	TRBE_FSC_SEA_LEVEL2	= 22,
> +	TRBE_FSC_SEA_LEVEL3	= 23,
> +	TRBE_FSC_ALIGN_FAULT	= 33,
> +	TRBE_FSC_TLB_FAULT	= 48,
> +	TRBE_FSC_ATOMIC_FAULT	= 49,
> +};

Please remove ^^^

> +
> +static const char *const trbe_fsc_str[] = {
> +	[TRBE_FSC_ASF_LEVEL0]	= "Address size fault - level 0",
> +	[TRBE_FSC_ASF_LEVEL1]	= "Address size fault - level 1",
> +	[TRBE_FSC_ASF_LEVEL2]	= "Address size fault - level 2",
> +	[TRBE_FSC_ASF_LEVEL3]	= "Address size fault - level 3",
> +	[TRBE_FSC_TF_LEVEL0]	= "Translation fault - level 0",
> +	[TRBE_FSC_TF_LEVEL1]	= "Translation fault - level 1",
> +	[TRBE_FSC_TF_LEVEL2]	= "Translation fault - level 2",
> +	[TRBE_FSC_TF_LEVEL3]	= "Translation fault - level 3",
> +	[TRBE_FSC_AFF_LEVEL0]	= "Access flag fault - level 0",
> +	[TRBE_FSC_AFF_LEVEL1]	= "Access flag fault - level 1",
> +	[TRBE_FSC_AFF_LEVEL2]	= "Access flag fault - level 2",
> +	[TRBE_FSC_AFF_LEVEL3]	= "Access flag fault - level 3",
> +	[TRBE_FSC_PF_LEVEL0]	= "Permission fault - level 0",
> +	[TRBE_FSC_PF_LEVEL1]	= "Permission fault - level 1",
> +	[TRBE_FSC_PF_LEVEL2]	= "Permission fault - level 2",
> +	[TRBE_FSC_PF_LEVEL3]	= "Permission fault - level 3",
> +	[TRBE_FSC_SEA_WRITE]	= "Synchronous external abort on write",
> +	[TRBE_FSC_ASEA_WRITE]	= "Asynchronous external abort on write",
> +	[TRBE_FSC_SEA_LEVEL0]	= "Syncrhonous external abort on table walk - level 0",
> +	[TRBE_FSC_SEA_LEVEL1]	= "Syncrhonous external abort on table walk - level 1",
> +	[TRBE_FSC_SEA_LEVEL2]	= "Syncrhonous external abort on table walk - level 2",
> +	[TRBE_FSC_SEA_LEVEL3]	= "Syncrhonous external abort on table walk - level 3",
> +	[TRBE_FSC_ALIGN_FAULT]	= "Alignment fault",
> +	[TRBE_FSC_TLB_FAULT]	= "TLB conflict fault",
> +	[TRBE_FSC_ATOMIC_FAULT]	= "Atmoc fault",
> +};
> 

Please remove ^^^

>

> +enum trbe_address_mode {
> +	TRBE_ADDRESS_VIRTUAL,
> +	TRBE_ADDRESS_PHYSICAL,
> +};

#define please.

> +
> +static const char *const trbe_address_mode_str[] = {
> +	[TRBE_ADDRESS_VIRTUAL]	= "Address mode - virtual",
> +	[TRBE_ADDRESS_PHYSICAL]	= "Address mode - physical",
> +};

Do we need this ? We always use virtual.

> +
> +static inline bool is_trbe_virtual_mode(void)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	return !(trblimitr & TRBLIMITR_NVM);
> +}
> +

Remove

> +static inline bool is_trbe_physical_mode(void)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	return trblimitr & TRBLIMITR_NVM;
> +}

Remove

> +
> +static inline void set_trbe_virtual_mode(void)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	trblimitr &= ~TRBLIMITR_NVM;
> +	write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
> +}
> +

> +static inline void set_trbe_physical_mode(void)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	trblimitr |= TRBLIMITR_NVM;
> +	write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
> +}

Remove

> +
> +enum trbe_trig_mode {
> +	TRBE_TRIGGER_STOP	= 0,
> +	TRBE_TRIGGER_IRQ	= 1,
> +	TRBE_TRIGGER_IGNORE	= 3,
> +};
> +
> +static const char *const trbe_trig_mode_str[] = {
> +	[TRBE_TRIGGER_STOP]	= "Trigger mode - stop",
> +	[TRBE_TRIGGER_IRQ]	= "Trigger mode - irq",
> +	[TRBE_TRIGGER_IGNORE]	= "Trigger mode - ignore",
> +};
> +
> +static inline enum trbe_trig_mode get_trbe_trig_mode(void)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	return (trblimitr >> TRBLIMITR_TRIG_MODE_SHIFT) & TRBLIMITR_TRIG_MODE_MASK;
> +}
> +
> +static inline void set_trbe_trig_mode(enum trbe_trig_mode mode)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	trblimitr &= ~(TRBLIMITR_TRIG_MODE_MASK << TRBLIMITR_TRIG_MODE_SHIFT);
> +	trblimitr |= ((mode & TRBLIMITR_TRIG_MODE_MASK) << TRBLIMITR_TRIG_MODE_SHIFT);
> +	write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
> +}
> +
> +enum trbe_fill_mode {
> +	TRBE_FILL_STOP		= 0,
> +	TRBE_FILL_WRAP		= 1,
> +	TRBE_FILL_CIRCULAR	= 3,
> +};
> +

Please use #define

> +static const char *const trbe_fill_mode_str[] = {
> +	[TRBE_FILL_STOP]	= "Buffer mode - stop",
> +	[TRBE_FILL_WRAP]	= "Buffer mode - wrap",
> +	[TRBE_FILL_CIRCULAR]	= "Buffer mode - circular",
> +};
> +
> +static inline enum trbe_fill_mode get_trbe_fill_mode(void)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	return (trblimitr >> TRBLIMITR_FILL_MODE_SHIFT) & TRBLIMITR_FILL_MODE_MASK;
> +}
> +
> +static inline void set_trbe_fill_mode(enum trbe_fill_mode mode)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	trblimitr &= ~(TRBLIMITR_FILL_MODE_MASK << TRBLIMITR_FILL_MODE_SHIFT);
> +	trblimitr |= ((mode & TRBLIMITR_FILL_MODE_MASK) << TRBLIMITR_FILL_MODE_SHIFT);
> +	write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
> +}
> +
> +static inline void set_trbe_disabled(void)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	trblimitr &= ~TRBLIMITR_ENABLE;
> +	write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
> +}
> +
> +static inline void set_trbe_enabled(void)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	trblimitr |= TRBLIMITR_ENABLE;
> +	write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
> +}
> +
> +static inline bool get_trbe_flag_update(void)
> +{
> +	u64 trbidr = read_sysreg_s(SYS_TRBIDR_EL1);
> +
> +	return trbidr & TRBIDR_FLAG;
> +}
> +
> +static inline bool is_trbe_programmable(void)
> +{
> +	u64 trbidr = read_sysreg_s(SYS_TRBIDR_EL1);
> +
> +	return !(trbidr & TRBIDR_PROG);
> +}
> +#
> +enum trbe_buffer_align {
> +	TRBE_BUFFER_BYTE,
> +	TRBE_BUFFER_HALF_WORD,
> +	TRBE_BUFFER_WORD,
> +	TRBE_BUFFER_DOUBLE_WORD,
> +	TRBE_BUFFER_16_BYTES,
> +	TRBE_BUFFER_32_BYTES,
> +	TRBE_BUFFER_64_BYTES,
> +	TRBE_BUFFER_128_BYTES,
> +	TRBE_BUFFER_256_BYTES,
> +	TRBE_BUFFER_512_BYTES,
> +	TRBE_BUFFER_1K_BYTES,
> +	TRBE_BUFFER_2K_BYTES,
> +};
> +

Remove ^^

> +static const char *const trbe_buffer_align_str[] = {
> +	[TRBE_BUFFER_BYTE]		= "Byte",
> +	[TRBE_BUFFER_HALF_WORD]		= "Half word",
> +	[TRBE_BUFFER_WORD]		= "Word",
> +	[TRBE_BUFFER_DOUBLE_WORD]	= "Double word",
> +	[TRBE_BUFFER_16_BYTES]		= "16 bytes",
> +	[TRBE_BUFFER_32_BYTES]		= "32 bytes",
> +	[TRBE_BUFFER_64_BYTES]		= "64 bytes",
> +	[TRBE_BUFFER_128_BYTES]		= "128 bytes",
> +	[TRBE_BUFFER_256_BYTES]		= "256 bytes",
> +	[TRBE_BUFFER_512_BYTES]		= "512 bytes",
> +	[TRBE_BUFFER_1K_BYTES]		= "1K bytes",
> +	[TRBE_BUFFER_2K_BYTES]		= "2K bytes",
> +};

We don't need any of this. We could simply "<<" and get the
size.


> +
> +static inline enum trbe_buffer_align get_trbe_address_align(void)
> +{
> +	u64 trbidr = read_sysreg_s(SYS_TRBIDR_EL1);
> +
> +	return (trbidr >> TRBIDR_ALIGN_SHIFT) & TRBIDR_ALIGN_MASK;
> +}
> +
> +static inline void assert_trbe_address_mode(unsigned long addr)
> +{
> +	bool virt_addr = virt_addr_valid(addr) || is_vmalloc_addr((void *)addr);
> +	bool virt_mode = is_trbe_virtual_mode();
> +
> +	WARN_ON(addr && ((virt_addr && !virt_mode) || (!virt_addr && virt_mode)));
> +}

I am not sure if this is really helpful. You have to trust the kernel vmalloc().

> +
> +static inline void assert_trbe_address_align(unsigned long addr)
> +{
> +	unsigned long nr_bytes = 1ULL << get_trbe_address_align();
> +
> +	WARN_ON(addr & (nr_bytes - 1));
> +}
> +
> +static inline unsigned long get_trbe_write_pointer(void)
> +{
> +	u64 trbptr = read_sysreg_s(SYS_TRBPTR_EL1);
> +	unsigned long addr = (trbptr >> TRBPTR_PTR_SHIFT) & TRBPTR_PTR_MASK;
> +
> +	assert_trbe_address_mode(addr);
> +	assert_trbe_address_align(addr);
> +	return addr;
> +}
> +
> +static inline void set_trbe_write_pointer(unsigned long addr)
> +{
> +	WARN_ON(is_trbe_enabled());
> +	assert_trbe_address_mode(addr);
> +	assert_trbe_address_align(addr);
> +	addr = (addr >> TRBPTR_PTR_SHIFT) & TRBPTR_PTR_MASK;
> +	write_sysreg_s(addr, SYS_TRBPTR_EL1);
> +}
> +
> +static inline unsigned long get_trbe_limit_pointer(void)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +	unsigned long limit = (trblimitr >> TRBLIMITR_LIMIT_SHIFT) & TRBLIMITR_LIMIT_MASK;
> +	unsigned long addr = limit << TRBLIMITR_LIMIT_SHIFT;
> +
> +	WARN_ON(addr & (PAGE_SIZE - 1));
> +	assert_trbe_address_mode(addr);
> +	assert_trbe_address_align(addr);
> +	return addr;
> +}
> +
> +static inline void set_trbe_limit_pointer(unsigned long addr)
> +{
> +	u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> +
> +	WARN_ON(is_trbe_enabled());
> +	assert_trbe_address_mode(addr);
> +	assert_trbe_address_align(addr);
> +	WARN_ON(addr & ((1UL << TRBLIMITR_LIMIT_SHIFT) - 1));
> +	WARN_ON(addr & (PAGE_SIZE - 1));
> +	trblimitr &= ~(TRBLIMITR_LIMIT_MASK << TRBLIMITR_LIMIT_SHIFT);
> +	trblimitr |= (addr & PAGE_MASK);
> +	write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
> +}
> +
> +static inline unsigned long get_trbe_base_pointer(void)
> +{
> +	u64 trbbaser = read_sysreg_s(SYS_TRBBASER_EL1);
> +	unsigned long addr = (trbbaser >> TRBBASER_BASE_SHIFT) & TRBBASER_BASE_MASK;
> +
> +	addr = addr << TRBBASER_BASE_SHIFT;
> +	WARN_ON(addr & (PAGE_SIZE - 1));
> +	assert_trbe_address_mode(addr);
> +	assert_trbe_address_align(addr);
> +	return addr;
> +}
> +
> +static inline void set_trbe_base_pointer(unsigned long addr)
> +{
> +	WARN_ON(is_trbe_enabled());
> +	assert_trbe_address_mode(addr);
> +	assert_trbe_address_align(addr);
> +	WARN_ON(addr & ((1UL << TRBBASER_BASE_SHIFT) - 1));
> +	WARN_ON(addr & (PAGE_SIZE - 1));
> +	write_sysreg_s(addr, SYS_TRBBASER_EL1);
> +}
> 

Suzuki