[PATCH 0/3] Provide ability to enable PSL traces on card probe

This patch-set updates the cxl adapter probe procedure to selectively enable
PSL-traces at the end of a successful probe for a PSL9 based adapter. This would
let implementors of AFUs that use in-kernel apis to interact with the CXL
adapter get early debug data.

This feature is controlled via a new module parameter named 'enable_psltrace'
which is enabled by default. During adapter probe is this parameter is set
cxl will start PSL-traces on the adapter via PSL_TRACECFG register. The needed
sequence values written to the register was provided by the PSL h/w team.

This patch set is based on an earlier patch titled
"cxl: Remove function write_timebase_ctrl_psl9() for PSL9" available at [1].

Ref: [1] http://patchwork.ozlabs.org/patch/871199/ 
Vaibhav Jain (3):
  cxl: Introduce various enums/defines for PSL9 trace arrays
  cxl: Introduce module parameter 'enable_psltrace'
  cxl: Provide implementation for sl_ops.start_psltrace on PSL9

 drivers/misc/cxl/cxl.h    | 31 ++++++++++++++--
 drivers/misc/cxl/main.c   |  4 +++
 drivers/misc/cxl/native.c |  8 ++---
 drivers/misc/cxl/pci.c    | 91 ++++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 119 insertions(+), 15 deletions(-)

-- 
2.14.3

We introduce a new enum named cxl_psl9_traceid that represents
individual trace-arrays available on PSL9. In addition a set of new
defines named s CXL_PSL9_TRACESTATE_XXX are introduced that represent
various states a trace-array can be in. Value of each define is the
value reported by PSL_CTCCFG register for the corresponding state of
trace-array. Also a new macro named CXL_PSL9_TRACE_STATE() is
introduced that makes it convenient to evaluate state of a trace-array
from the PSL_CTCCFG register.

The patch re-factors cxl_stop_trace_psl9() to use these new
enums/defines and renames 'cxl_service_layer_ops.debugfs_stoptrace'
function pointer to 'cxl_service_layer_ops.stop_psltrace'.

This patch shouldn't cause any behavioral changes.

Signed-off-by: Vaibhav Jain <redacted>
---
 drivers/misc/cxl/cxl.h    | 29 ++++++++++++++++++++++++++---
 drivers/misc/cxl/native.c |  8 ++++----
 drivers/misc/cxl/pci.c    | 15 +++++++--------
 3 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 4f015da78f28..81da307b60c0 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h

@@ -418,8 +418,31 @@ static const cxl_p2n_reg_t CXL_PSL_WED_An     = {0x0A0};
 #define CXL_CARD_MINOR(adapter) (adapter->adapter_num * CXL_DEV_MINORS)
 #define CXL_DEVT_ADAPTER(dev) (MINOR(dev) / CXL_DEV_MINORS)
 
-#define CXL_PSL9_TRACEID_MAX 0xAU
-#define CXL_PSL9_TRACESTATE_FIN 0x3U
+/* Various trace arrays available on PSL9 */
+enum cxl_psl9_traceid {
+	CXL_PSL9_TRACEID_RX0 = 0,
+	CXL_PSL9_TRACEID_RX1, /* 1 */
+	CXL_PSL9_TRACEID_XSL, /* 2 */
+	CXL_PSL9_TRACEID_CT0, /* 3 */
+	CXL_PSL9_TRACEID_CT1, /* 4 */
+	CXL_PSL9_TRACEID_LA0, /* 5 */
+	CXL_PSL9_TRACEID_LA1, /* 6 */
+	CXL_PSL9_TRACEID_JM0, /* 7 */
+	CXL_PSL9_TRACEID_DMA0, /* 8 */
+	CXL_PSL9_TRACEID_DMA1, /* 9 */
+	CXL_PSL9_TRACEID_REOA, /* 10 */
+	CXL_PSL9_TRACEID_MAX
+};
+
+/* State of tracearray as reported in PSL9_CTCCFG */
+#define CXL_PSL9_TRACESTATE_IDLE	0x0U
+#define CXL_PSL9_TRACESTATE_INIT	0x1U
+#define CXL_PSL9_TRACESTATE_WFT		0x2U
+#define CXL_PSL9_TRACESTATE_FIN		0x3U
+
+/* Find the Trace Array state from PSL9_CTCCFG Reg */
+#define CXL_PSL9_TRACE_STATE(STATE, TRACE_ID)		\
+	((STATE) >> (62 - (TRACE_ID) * 2) & 0x3)
 
 enum cxl_context_status {
 	CLOSED,

@@ -654,7 +677,7 @@ struct cxl_service_layer_ops {
 	void (*debugfs_add_afu_regs)(struct cxl_afu *afu, struct dentry *dir);
 	void (*psl_irq_dump_registers)(struct cxl_context *ctx);
 	void (*err_irq_dump_registers)(struct cxl *adapter);
-	void (*debugfs_stop_trace)(struct cxl *adapter);
+	void (*stop_psltrace)(struct cxl *adapter);
 	void (*write_timebase_ctrl)(struct cxl *adapter);
 	u64 (*timebase_read)(struct cxl *adapter);
 	int capi_mode;

diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 1b3d7c65ea3f..bba9e1bb8b4d 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c

@@ -1135,9 +1135,9 @@ static irqreturn_t native_handle_psl_slice_error(struct cxl_context *ctx,
 	if (ctx->afu->adapter->native->sl_ops->psl_irq_dump_registers)
 		ctx->afu->adapter->native->sl_ops->psl_irq_dump_registers(ctx);
 
-	if (ctx->afu->adapter->native->sl_ops->debugfs_stop_trace) {
+	if (ctx->afu->adapter->native->sl_ops->stop_psltrace) {
 		dev_crit(&ctx->afu->dev, "STOPPING CXL TRACE\n");
-		ctx->afu->adapter->native->sl_ops->debugfs_stop_trace(ctx->afu->adapter);
+		ctx->afu->adapter->native->sl_ops->stop_psltrace(ctx->afu->adapter);
 	}
 
 	return cxl_ops->ack_irq(ctx, 0, errstat);

@@ -1303,9 +1303,9 @@ static irqreturn_t native_irq_err(int irq, void *data)
 	err_ivte = cxl_p1_read(adapter, CXL_PSL_ErrIVTE);
 	dev_crit(&adapter->dev, "PSL_ErrIVTE: 0x%016llx\n", err_ivte);
 
-	if (adapter->native->sl_ops->debugfs_stop_trace) {
+	if (adapter->native->sl_ops->stop_psltrace) {
 		dev_crit(&adapter->dev, "STOPPING CXL TRACE\n");
-		adapter->native->sl_ops->debugfs_stop_trace(adapter);
+		adapter->native->sl_ops->stop_psltrace(adapter);
 	}
 
 	if (adapter->native->sl_ops->err_irq_dump_registers)

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 9bc30c20b66b..926b13973b73 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c

@@ -1747,15 +1747,14 @@ static void cxl_deconfigure_adapter(struct cxl *adapter)
 static void cxl_stop_trace_psl9(struct cxl *adapter)
 {
 	int traceid;
-	u64 trace_state, trace_mask;
+	u64 trace_cfg, trace_state;
 	struct pci_dev *dev = to_pci_dev(adapter->dev.parent);
 
+	trace_cfg = cxl_p1_read(adapter, CXL_PSL9_CTCCFG);
 	/* read each tracearray state and issue mmio to stop them is needed */
-	for (traceid = 0; traceid <= CXL_PSL9_TRACEID_MAX; ++traceid) {
-		trace_state = cxl_p1_read(adapter, CXL_PSL9_CTCCFG);
-		trace_mask = (0x3ULL << (62 - traceid * 2));
-		trace_state = (trace_state & trace_mask) >> (62 - traceid * 2);
-		dev_dbg(&dev->dev, "cxl: Traceid-%d trace_state=0x%0llX\n",
+	for (traceid = 0; traceid < CXL_PSL9_TRACEID_MAX; ++traceid) {
+		trace_state = CXL_PSL9_TRACE_STATE(trace_cfg, traceid);
+		dev_dbg(&dev->dev, "Traceid-%d trace_state=0x%0llX\n",
 			traceid, trace_state);
 
 		/* issue mmio if the trace array isn't in FIN state */

@@ -1799,7 +1798,7 @@ static const struct cxl_service_layer_ops psl9_ops = {
 	.debugfs_add_afu_regs = cxl_debugfs_add_afu_regs_psl9,
 	.psl_irq_dump_registers = cxl_native_irq_dump_regs_psl9,
 	.err_irq_dump_registers = cxl_native_err_irq_dump_regs_psl9,
-	.debugfs_stop_trace = cxl_stop_trace_psl9,
+	.stop_psltrace = cxl_stop_trace_psl9,
 	.timebase_read = timebase_read_psl9,
 	.capi_mode = OPAL_PHB_CAPI_MODE_CAPI,
 	.needs_reset_before_disable = true,

@@ -1822,7 +1821,7 @@ static const struct cxl_service_layer_ops psl8_ops = {
 	.debugfs_add_afu_regs = cxl_debugfs_add_afu_regs_psl8,
 	.psl_irq_dump_registers = cxl_native_irq_dump_regs_psl8,
 	.err_irq_dump_registers = cxl_native_err_irq_dump_regs_psl8,
-	.debugfs_stop_trace = cxl_stop_trace_psl8,
+	.stop_psltrace = cxl_stop_trace_psl8,
 	.write_timebase_ctrl = write_timebase_ctrl_psl8,
 	.timebase_read = timebase_read_psl8,
 	.capi_mode = OPAL_PHB_CAPI_MODE_CAPI,

-- 
2.14.3

We introduce a new module parameter named 'enable_psltrace' which asks cxl
to start(by default) psl-traces on an adapter as soon as its probe is
finished. In case this default behavior is not needed then this
module parameter can be set to '0'.

Signed-off-by: Vaibhav Jain <redacted>
---
 drivers/misc/cxl/cxl.h  | 2 ++
 drivers/misc/cxl/main.c | 4 ++++
 drivers/misc/cxl/pci.c  | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 81da307b60c0..1af66451cbb4 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h

@@ -28,6 +28,7 @@
 #include <uapi/misc/cxl.h>
 
 extern uint cxl_verbose;
+extern bool cxl_enable_psltrace;
 
 #define CXL_TIMEOUT 5
 

@@ -678,6 +679,7 @@ struct cxl_service_layer_ops {
 	void (*psl_irq_dump_registers)(struct cxl_context *ctx);
 	void (*err_irq_dump_registers)(struct cxl *adapter);
 	void (*stop_psltrace)(struct cxl *adapter);
+	void (*start_psltrace)(struct cxl *adapter);
 	void (*write_timebase_ctrl)(struct cxl *adapter);
 	u64 (*timebase_read)(struct cxl *adapter);
 	int capi_mode;

diff --git a/drivers/misc/cxl/main.c b/drivers/misc/cxl/main.c
index c1ba0d42cbc8..593f2cdba3d8 100644
--- a/drivers/misc/cxl/main.c
+++ b/drivers/misc/cxl/main.c

@@ -34,6 +34,10 @@ uint cxl_verbose;
 module_param_named(verbose, cxl_verbose, uint, 0600);
 MODULE_PARM_DESC(verbose, "Enable verbose dmesg output");
 
+bool cxl_enable_psltrace = true;
+module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
+MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: on");
+
 const struct cxl_backend_ops *cxl_ops;
 
 int cxl_afu_slbia(struct cxl_afu *afu)

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 926b13973b73..9e8b8525534c 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c

@@ -1726,6 +1726,9 @@ static int cxl_configure_adapter(struct cxl *adapter, struct pci_dev *dev)
 	if ((rc = cxl_native_register_psl_err_irq(adapter)))
 		goto err;
 
+	if (cxl_enable_psltrace && adapter->native->sl_ops->start_psltrace)
+		adapter->native->sl_ops->start_psltrace(adapter);
+
 	return 0;
 
 err:

-- 
2.14.3

We introduce a new function named cxl_start_trace_psl9() that starts
the various trace-arrays available on PSL9. The implementation
configures trace-array-units(TAU) via multiple writes to the
PSL_TRACECFG register and uses the defaults (data-bus, trigger-bus &
trigger-masks) provided by the h/w team for each trace-array. These
defaults are defined in array cxl_psl_trace_mask.

The implementation takes care of not re-initializing a trace-array in
case its already in 'FIN' or 'WFT' or 'INIT' state so as to not reset
the existing trace data. After moving the individual trace-arrays to
'INIT' state we poll the PSL_CTCCFG register to ensure the all
configured TAUs to move to WFT/FIN state so that no traces/triggers
are missed.

Signed-off-by: Vaibhav Jain <redacted>
---
 drivers/misc/cxl/pci.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 9e8b8525534c..44b1843c5b7a 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c

@@ -1767,6 +1767,78 @@ static void cxl_stop_trace_psl9(struct cxl *adapter)
 	}
 }
 
+/* Default enable mask for various trace arrays */
+static const uint64_t cxl_psl_trace_mask[] = {
+	[CXL_PSL9_TRACEID_RX0]  = 0x8200000000000000ULL,
+	[CXL_PSL9_TRACEID_RX1]  = 0xAA00000000000001UL,
+	[CXL_PSL9_TRACEID_XSL]  = 0x0,
+	[CXL_PSL9_TRACEID_CT0]  = 0xA2B8000000000003UL,
+	[CXL_PSL9_TRACEID_CT1]  = 0x0,
+	[CXL_PSL9_TRACEID_LA0]  = 0x83FFC00000000005ULL,
+	[CXL_PSL9_TRACEID_LA1]  = 0x83FFC00000000006ULL,
+	[CXL_PSL9_TRACEID_JM0]  = 0x8200000000000007ULL,
+	[CXL_PSL9_TRACEID_DMA0] = 0x8200000000000008UL,
+	[CXL_PSL9_TRACEID_DMA1] = 0x8200000000000009UL,
+	[CXL_PSL9_TRACEID_REOA] = 0x0,
+};
+
+static void cxl_start_trace_psl9(struct cxl *adapter)
+{
+	int traceid;
+	unsigned long timeout;
+	struct pci_dev *dev = to_pci_dev(adapter->dev.parent);
+	uint64_t trace_mask, trace_cfg = cxl_p1_read(adapter, CXL_PSL9_CTCCFG);
+
+	dev_dbg(&dev->dev, "Enabling traces. PSL_CTCCFG=0x%016llx\n",
+		trace_cfg);
+
+	for (trace_mask = traceid = 0;
+	     traceid < CXL_PSL9_TRACEID_MAX; ++traceid) {
+
+		if (cxl_psl_trace_mask[traceid] == 0)
+			continue;
+
+		if (CXL_PSL9_TRACE_STATE(trace_cfg, traceid) ==
+		    CXL_PSL9_TRACESTATE_IDLE) {
+			dev_dbg(&dev->dev, "Traceid-%d Initializing\n",
+				traceid);
+			cxl_p1_write(adapter, CXL_PSL9_TRACECFG,
+				     cxl_psl_trace_mask[traceid]);
+
+			/* filter out tlbie-response */
+			if (traceid == CXL_PSL9_TRACEID_LA0) {
+				cxl_p1_write(adapter, CXL_PSL9_TRACECFG,
+					     0x81FF400000000005ULL);
+			}
+
+			/* Mask so that we can poll for exit from INIT state */
+			trace_mask |= CXL_PSL9_TRACESTATE_WFT <<
+				(62 - traceid * 2);
+		} else {
+			dev_dbg(&dev->dev, "Traceid-%d already init.\n",
+				traceid);
+		}
+	}
+
+	/* poll until all the enabled arrays are out of INIT state */
+	timeout = jiffies + (HZ * CXL_TIMEOUT);
+	while (time_before(jiffies, timeout)) {
+		trace_cfg = cxl_p1_read(adapter, CXL_PSL9_CTCCFG);
+		if ((trace_cfg & trace_mask) == trace_mask)
+			break;
+		schedule();
+	}
+
+	if ((trace_cfg & trace_mask) != trace_mask) {
+		dev_warn(&dev->dev, "Trace init timeout."
+			 "Some trace events may be missed.\n");
+		dev_dbg(&dev->dev, "cxl:CTCCFG=0x%016llx\n", trace_cfg);
+
+	} else {
+		dev_info(&dev->dev, "Traces enabled\n");
+	}
+}
+
 static void cxl_stop_trace_psl8(struct cxl *adapter)
 {
 	int slice;

@@ -1802,6 +1874,7 @@ static const struct cxl_service_layer_ops psl9_ops = {
 	.psl_irq_dump_registers = cxl_native_irq_dump_regs_psl9,
 	.err_irq_dump_registers = cxl_native_err_irq_dump_regs_psl9,
 	.stop_psltrace = cxl_stop_trace_psl9,
+	.start_psltrace = cxl_start_trace_psl9,
 	.timebase_read = timebase_read_psl9,
 	.capi_mode = OPAL_PHB_CAPI_MODE_CAPI,
 	.needs_reset_before_disable = true,

-- 
2.14.3

Le 09/02/2018 à 05:25, Vaibhav Jain a écrit :

any reason to use dev_dbg instead of pr_devel ?

Le 09/02/2018 à 05:25, Vaibhav Jain a écrit :

I am not too agree to add a new parameter. This can cause doubts.
PSL team has confirmed that enabling traces has no impact.
Do you see any reason to disable the traces ?

Thanks for reviewing the patch Christophe,
christophe lombard [off-list ref] writes:

quoted

+	for (traceid = 0; traceid < CXL_PSL9_TRACEID_MAX; ++traceid) {
+		trace_state = CXL_PSL9_TRACE_STATE(trace_cfg, traceid);
+		dev_dbg(&dev->dev, "Traceid-%d trace_state=0x%0llX\n",
  			traceid, trace_state);

any reason to use dev_dbg instead of pr_devel ?

Wanted to distinguish among multiple cxl cards in the system.

-- 
Vaibhav Jain [off-list ref]
Linux Technology Center, IBM India Pvt. Ltd.

Thanks for reviewing the patch Christophe,

christophe lombard [off-list ref] writes:

quoted

+bool cxl_enable_psltrace = true;
+module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
+MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: on");
+

I am not too agree to add a new parameter. This can cause doubts.
PSL team has confirmed that enabling traces has no impact.
Do you see any reason to disable the traces ?

Traces on PSL follow a 'set and fetch' model. So once the trace buffer for
a specific array is full it will stop and switch to 'FIN' state and at
that point we need to fetch the trace-data and reinit the array to
re-arm it.

There might be some circumstances where this model may lead to confusion
specifically when AFU developers assume that the trace arrays are
already armed and dont re-arm it causing miss of trace data.

So this module param is a compromise to keep the old behaviour of traces
array intact where in the arming/disarming of the trace arrays is
controlled completely by userspace tooling and not by cxl.

-- 
Vaibhav Jain [off-list ref]
Linux Technology Center, IBM India Pvt. Ltd.

Le 11/02/2018 à 18:10, Vaibhav Jain a écrit :

Thanks for reviewing the patch Christophe,

christophe lombard [off-list ref] writes:

quoted

quoted

+bool cxl_enable_psltrace = true;
+module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
+MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: on");
+

I am not too agree to add a new parameter. This can cause doubts.
PSL team has confirmed that enabling traces has no impact.
Do you see any reason to disable the traces ?

Traces on PSL follow a 'set and fetch' model. So once the trace buffer for
a specific array is full it will stop and switch to 'FIN' state and at
that point we need to fetch the trace-data and reinit the array to
re-arm it.

There might be some circumstances where this model may lead to confusion
specifically when AFU developers assume that the trace arrays are
already armed and dont re-arm it causing miss of trace data.

So this module param is a compromise to keep the old behaviour of traces
array intact where in the arming/disarming of the trace arrays is
controlled completely by userspace tooling and not by cxl.

and about P8 ? This new parameter is only useful for P9. It will be 
confusing.

Le 11/02/2018 à 18:10, Vaibhav Jain a écrit :

Thanks for reviewing the patch Christophe,

christophe lombard [off-list ref] writes:

quoted

quoted

+bool cxl_enable_psltrace = true;
+module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
+MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: on");
+

I am not too agree to add a new parameter. This can cause doubts.
PSL team has confirmed that enabling traces has no impact.
Do you see any reason to disable the traces ?

Traces on PSL follow a 'set and fetch' model. So once the trace buffer for
a specific array is full it will stop and switch to 'FIN' state and at
that point we need to fetch the trace-data and reinit the array to
re-arm it.

If the PSL trace arrays don't wrap, is there anything to gain by 
enabling tracing by default instead of letting the developer handle it 
through sysfs? I was under the (now wrong) impression that the PSL would 
wrap.
I'm not a big fan of the module parameter. It seems we're giving a 
second way of activating traces on top of sysfs, more cumbersome and 
limited.

   Fred

There might be some circumstances where this model may lead to confusion
specifically when AFU developers assume that the trace arrays are
already armed and dont re-arm it causing miss of trace data.

So this module param is a compromise to keep the old behaviour of traces
array intact where in the arming/disarming of the trace arrays is
controlled completely by userspace tooling and not by cxl.

Frederic Barrat [off-list ref] writes:

Le 11/02/2018 =C3=A0 18:10, Vaibhav Jain a =C3=A9crit=C2=A0:

quoted

Thanks for reviewing the patch Christophe,
=20
christophe lombard [off-list ref] writes:

quoted

quoted

+bool cxl_enable_psltrace =3D true;
+module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
+MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: =

on");

quoted

quoted

quoted

+

I am not too agree to add a new parameter. This can cause doubts.
PSL team has confirmed that enabling traces has no impact.
Do you see any reason to disable the traces ?

=20
Traces on PSL follow a 'set and fetch' model. So once the trace buffer f=

or

quoted

a specific array is full it will stop and switch to 'FIN' state and at
that point we need to fetch the trace-data and reinit the array to
re-arm it.

If the PSL trace arrays don't wrap, is there anything to gain by=20
enabling tracing by default instead of letting the developer handle it=20
through sysfs? I was under the (now wrong) impression that the PSL would=

=20

wrap.

Enabling the traces quickly enough should let AFU developers debug init
issues. Specifically AFU's that rely on cxl kernel-apis.

I'm not a big fan of the module parameter. It seems we're giving a=20
second way of activating traces on top of sysfs, more cumbersome and=20
limited.

Yes, this indeed is providing a second way of activating traces on top
of sysfs. The way I see this that there are two ways PSL traces are
managed:

1. Let userspace handle state machine of the traces entirely via sysfs.
2. PSL trace machine is handled via cxl. It starts it when a card is
probed and stops it when the card is reset.

--=20
Vaibhav Jain [off-list ref]
Linux Technology Center, IBM India Pvt. Ltd.