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

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

[Vaibhav Jain]

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

[PATCH 1/3] cxl: Introduce various enums/defines for PSL9 trace arrays

[Vaibhav Jain]

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 <vaibhav@linux.vnet.ibm.com>
---
 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

[PATCH 2/3] cxl: Introduce module parameter 'enable_psltrace'

[Vaibhav Jain]

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 <vaibhav@linux.vnet.ibm.com>
---
 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

[PATCH 3/3] cxl: Provide implementation for sl_ops.start_psltrace on PSL9

[Vaibhav Jain]

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 <vaibhav@linux.vnet.ibm.com>
---
 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

Re: [PATCH 1/3] cxl: Introduce various enums/defines for PSL9 trace arrays

[christophe lombard]

Le 09/02/2018 à 05:25, Vaibhav Jain a écrit :
> 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 <vaibhav@linux.vnet.ibm.com>
> ---
>   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);
> 
any reason to use dev_dbg instead of pr_devel ?

>   		/* 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,
> 

Re: [PATCH 2/3] cxl: Introduce module parameter 'enable_psltrace'

[christophe lombard]

Le 09/02/2018 à 05:25, Vaibhav Jain a écrit :
> 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 <vaibhav@linux.vnet.ibm.com>
> ---
>   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");
> +
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 ?

>   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:
> 

Re: [PATCH 1/3] cxl: Introduce various enums/defines for PSL9 trace arrays

[Vaibhav Jain]

Thanks for reviewing the patch Christophe,
christophe lombard <clombard@linux.vnet.ibm.com> writes:

>> +	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 <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH 2/3] cxl: Introduce module parameter 'enable_psltrace'

[Vaibhav Jain]

Thanks for reviewing the patch Christophe,

christophe lombard <clombard@linux.vnet.ibm.com> writes:
>> +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 <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH 2/3] cxl: Introduce module parameter 'enable_psltrace'

[christophe lombard]

Le 11/02/2018 à 18:10, Vaibhav Jain a écrit :
> Thanks for reviewing the patch Christophe,
> 
> christophe lombard <clombard@linux.vnet.ibm.com> writes:
>>> +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.

Re: [PATCH 2/3] cxl: Introduce module parameter 'enable_psltrace'

[Frederic Barrat]

Le 11/02/2018 à 18:10, Vaibhav Jain a écrit :
> Thanks for reviewing the patch Christophe,
> 
> christophe lombard <clombard@linux.vnet.ibm.com> writes:
>>> +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.
> 

Re: [PATCH 2/3] cxl: Introduce module parameter 'enable_psltrace'

[Vaibhav Jain]

Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes:

> Le 11/02/2018 =C3=A0 18:10, Vaibhav Jain a =C3=A9crit=C2=A0:
>> Thanks for reviewing the patch Christophe,
>>=20
>> christophe lombard <clombard@linux.vnet.ibm.com> writes:
>>>> +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");
>>>> +
>>> 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
>> 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 <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.