[PATCH v2 0/4] usb: typec: tps6598x: Add IRQ flag and register tracing

[PATCH v2 0/4] usb: typec: tps6598x: Add IRQ flag and register tracing

[Guido Günther]

This series adds tracing events for the chips IRQ and registers that are useful
to figure out the current data and power status. This came about since
diagnosing why a certain usb-c hub or dp-alt-mode adapter fails is hard with
the information in /sys/class/typec alone since this does not have a timeline
of events (and we don't want every typec user having to also buy a PD
analyzer). With this series debugging these kinds of things starts to become
fun:

   # echo 1 > /sys/kernel/debug/tracing/events/tps6598x/enable
   # cat /sys/kernel/debug/tracing/trace_pipe
   irq/79-0-003f-526     [003] ....   512.717871: tps6598x_irq: event1=PLUG_EVENT|DATA_STATUS_UPDATE|STATUS_UPDATE, event2=
   irq/79-0-003f-526     [003] ....   512.722408: tps6598x_status: conn: conn-Ra, pp_5v0: off, pp_hv: off, pp_ext: off, pp_cable: off, pwr-src: vin-3p3, vbus: vSafe0V, usb-host: no, legacy: no, flags: PLUG_PRESENT|PORTROLE|DATAROLE
   irq/79-0-003f-526     [003] ....   512.727127: tps6598x_data_status: DATA_CONNECTION|USB2_CONNECTION|USB3_CONNECTION
   irq/79-0-003f-526     [003] ....   512.769571: tps6598x_irq: event1=PP_SWITCH_CHANGED|STATUS_UPDATE, event2=
   irq/79-0-003f-526     [003] ....   512.773380: tps6598x_status: conn: conn-Ra, pp_5v0: out, pp_hv: off, pp_ext: off, pp_cable: in, pwr-src: vin-3p3, vbus: vSafe0V, usb-host: no, legacy: no, flags: PLUG_PRESENT|PORTROLE|DATAROLE|VCONN
   irq/79-0-003f-526     [003] ....   512.872450: tps6598x_irq: event1=POWER_STATUS_UPDATE|PD_STATUS_UPDATE, event2=
   irq/79-0-003f-526     [003] ....   512.876311: tps6598x_status: conn: conn-Ra, pp_5v0: out, pp_hv: off, pp_ext: off, pp_cable: in, pwr-src: vin-3p3, vbus: vSafe0V, usb-host: no, legacy: no, flags: PLUG_PRESENT|PORTROLE|DATAROLE|VCONN
   irq/79-0-003f-526     [003] ....   512.880237: tps6598x_power_status: conn: 1, pwr-role: source, typec: usb, bc: sdp
   irq/79-0-003f-526     [003] ....   513.072682: tps6598x_irq: event1=STATUS_UPDATE, event2=
   irq/79-0-003f-526     [003] ....   513.076390: tps6598x_status: conn: conn-Ra, pp_5v0: out, pp_hv: off, pp_ext: off, pp_cable: in, pwr-src: vin-3p3, vbus: vSafe5V, usb-host: no, legacy: no, flags: PLUG_PRESENT|PORTROLE|DATAROLE|VCONN
   irq/79-0-003f-526     [003] ....   513.090676: tps6598x_irq: event1=ERROR_CANNOT_PROVIDE_PWR, event2=
   irq/79-0-003f-526     [003] ....   513.094368: tps6598x_status: conn: conn-Ra, pp_5v0: out, pp_hv: off, pp_ext: off, pp_cable: in, pwr-src: vin-3p3, vbus: vSafe5V, usb-host: no, legacy: no, flags: PLUG_PRESENT|PORTROLE|DATAROLE|VCONN
   irq/79-0-003f-526     [003] ....   513.109606: tps6598x_irq: event1=NEW_CONTRACT_AS_PROVIDER|POWER_STATUS_UPDATE|STATUS_UPDATE|SRC_TRANSITION, event2=
   irq/79-0-003f-526     [003] ....   513.113777: tps6598x_status: conn: conn-Ra, pp_5v0: out, pp_hv: off, pp_ext: off, pp_cable: in, pwr-src: vin-3p3, vbus: pd, usb-host: no, legacy: no, flags: PLUG_PRESENT|PORTROLE|DATAROLE|VCONN
   irq/79-0-003f-526     [003] ....   513.117475: tps6598x_power_status: conn: 1, pwr-role: source, typec: pd, bc: sdp
   irq/79-0-003f-526     [003] ....   513.137469: tps6598x_irq: event1=VDM_RECEIVED, event2=
   irq/79-0-003f-526     [003] ....   513.141570: tps6598x_status: conn: conn-Ra, pp_5v0: out, pp_hv: off, pp_ext: off, pp_cable: in, pwr-src: vin-3p3, vbus: pd, usb-host: no, legacy: no, flags: PLUG_PRESENT|PORTROLE|DATAROLE|VCONN
   irq/79-0-003f-526     [003] ....   513.281926: tps6598x_irq: event1=VDM_RECEIVED, event2=
   irq/79-0-003f-526     [003] ....   513.285638: tps6598x_status: conn: conn-Ra, pp_5v0: out, pp_hv: off, pp_ext: off, pp_cable: in, pwr-src: vin-3p3, vbus: pd, usb-host: no, legacy: no, flags: PLUG_PRESENT|PORTROLE|DATAROLE|VCONN
   irq/79-0-003f-526     [003] ....   513.300515: tps6598x_irq: event1=VDM_RECEIVED|DATA_STATUS_UPDATE, event2=
   irq/79-0-003f-526     [003] ....   513.304226: tps6598x_status: conn: conn-Ra, pp_5v0: out, pp_hv: off, pp_ext: off, pp_cable: in, pwr-src: vin-3p3, vbus: pd, usb-host: no, legacy: no, flags: PLUG_PRESENT|PORTROLE|DATAROLE|VCONN
   irq/79-0-003f-526     [003] ....   513.308302: tps6598x_data_status: DATA_CONNECTION|USB2_CONNECTION|USB3_CONNECTION|DP_CONNECTION, DP pinout D

It should not impose any problems for firmwares that don't have IRQs for the
registers enabled. The trace will then just be missing those events.

Patch is against next-20210210.

changes from v1:
- Fix issues on 32bit arches and missing include spotted by kbuild
  (build testes on mips)



Guido Günther (4):
  usb: typec: tps6598x: Add trace event for IRQ events
  usb: typec: tps6598x: Add trace event for status register
  usb: typec: tps6598x: Add trace event for power status register
  usb: typec: tps6598x: Add trace event for data status

 drivers/usb/typec/Makefile         |   3 +
 drivers/usb/typec/tps6598x.c       |  66 ++++---
 drivers/usb/typec/tps6598x.h       | 185 +++++++++++++++++++
 drivers/usb/typec/tps6598x_trace.h | 283 +++++++++++++++++++++++++++++
 4 files changed, 511 insertions(+), 26 deletions(-)
 create mode 100644 drivers/usb/typec/tps6598x.h
 create mode 100644 drivers/usb/typec/tps6598x_trace.h

-- 
2.30.0

[PATCH v2 1/4] usb: typec: tps6598x: Add trace event for IRQ events

[Guido Günther]

Allow to get irq event information via the tracing framework.  This
allows to inspect USB-C negotiation at runtime.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/usb/typec/Makefile         |  3 +
 drivers/usb/typec/tps6598x.c       |  9 ++-
 drivers/usb/typec/tps6598x.h       | 64 ++++++++++++++++++++
 drivers/usb/typec/tps6598x_trace.h | 97 ++++++++++++++++++++++++++++++
 4 files changed, 170 insertions(+), 3 deletions(-)
 create mode 100644 drivers/usb/typec/tps6598x.h
 create mode 100644 drivers/usb/typec/tps6598x_trace.h

diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index d03b48c4b864..27aa12129190 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1,4 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
+# define_trace.h needs to know how to find our header
+CFLAGS_tps6598x.o		:= -I$(src)
+
 obj-$(CONFIG_TYPEC)		+= typec.o
 typec-y				:= class.o mux.o bus.o
 obj-$(CONFIG_TYPEC)		+= altmodes/
diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index 6e6ef6317523..bc34b35e909f 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -6,6 +6,8 @@
  * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
  */
 
+#include "tps6598x.h"
+
 #include <linux/i2c.h>
 #include <linux/acpi.h>
 #include <linux/module.h>
@@ -15,6 +17,9 @@
 #include <linux/usb/typec.h>
 #include <linux/usb/role.h>
 
+#define CREATE_TRACE_POINTS
+#include "tps6598x_trace.h"
+
 /* Register offsets */
 #define TPS_REG_VID			0x00
 #define TPS_REG_MODE			0x03
@@ -32,9 +37,6 @@
 #define TPS_REG_POWER_STATUS		0x3f
 #define TPS_REG_RX_IDENTITY_SOP		0x48
 
-/* TPS_REG_INT_* bits */
-#define TPS_REG_INT_PLUG_EVENT		BIT(3)
-
 /* TPS_REG_STATUS bits */
 #define TPS_STATUS_PLUG_PRESENT		BIT(0)
 #define TPS_STATUS_ORIENTATION		BIT(4)
@@ -428,6 +430,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 		dev_err(tps->dev, "%s: failed to read events\n", __func__);
 		goto err_unlock;
 	}
+	trace_tps6598x_irq(event1, event2);
 
 	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
 	if (ret) {
diff --git a/drivers/usb/typec/tps6598x.h b/drivers/usb/typec/tps6598x.h
new file mode 100644
index 000000000000..b83b8a6a1504
--- /dev/null
+++ b/drivers/usb/typec/tps6598x.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Driver for TI TPS6598x USB Power Delivery controller family
+ *
+ * Copyright (C) 2017, Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+
+#ifndef __TPS6598X_H__
+#define __TPS6598X_H__
+
+
+/* TPS_REG_INT_* bits */
+#define TPS_REG_INT_USER_VID_ALT_MODE_OTHER_VDM		BIT_ULL(27+32)
+#define TPS_REG_INT_USER_VID_ALT_MODE_ATTN_VDM		BIT_ULL(26+32)
+#define TPS_REG_INT_USER_VID_ALT_MODE_EXIT		BIT_ULL(25+32)
+#define TPS_REG_INT_USER_VID_ALT_MODE_ENTERED		BIT_ULL(24+32)
+#define TPS_REG_INT_EXIT_MODES_COMPLETE			BIT_ULL(20+32)
+#define TPS_REG_INT_DISCOVER_MODES_COMPLETE		BIT_ULL(19+32)
+#define TPS_REG_INT_VDM_MSG_SENT			BIT_ULL(18+32)
+#define TPS_REG_INT_VDM_ENTERED_MODE			BIT_ULL(17+32)
+#define TPS_REG_INT_ERROR_UNABLE_TO_SOURCE		BIT_ULL(14+32)
+#define TPS_REG_INT_SRC_TRANSITION			BIT_ULL(10+32)
+#define TPS_REG_INT_ERROR_DISCHARGE_FAILED		BIT_ULL(9+32)
+#define TPS_REG_INT_ERROR_MESSAGE_DATA			BIT_ULL(7+32)
+#define TPS_REG_INT_ERROR_PROTOCOL_ERROR		BIT_ULL(6+32)
+#define TPS_REG_INT_ERROR_MISSING_GET_CAP_MESSAGE	BIT_ULL(4+32)
+#define TPS_REG_INT_ERROR_POWER_EVENT_OCCURRED		BIT_ULL(3+32)
+#define TPS_REG_INT_ERROR_CAN_PROVIDE_PWR_LATER		BIT_ULL(2+32)
+#define TPS_REG_INT_ERROR_CANNOT_PROVIDE_PWR		BIT_ULL(1+32)
+#define TPS_REG_INT_ERROR_DEVICE_INCOMPATIBLE		BIT_ULL(0+32)
+#define TPS_REG_INT_CMD2_COMPLETE			BIT(31)
+#define TPS_REG_INT_CMD1_COMPLETE			BIT(30)
+#define TPS_REG_INT_ADC_HIGH_THRESHOLD			BIT(29)
+#define TPS_REG_INT_ADC_LOW_THRESHOLD			BIT(28)
+#define TPS_REG_INT_PD_STATUS_UPDATE			BIT(27)
+#define TPS_REG_INT_STATUS_UPDATE			BIT(26)
+#define TPS_REG_INT_DATA_STATUS_UPDATE			BIT(25)
+#define TPS_REG_INT_POWER_STATUS_UPDATE			BIT(24)
+#define TPS_REG_INT_PP_SWITCH_CHANGED			BIT(23)
+#define TPS_REG_INT_HIGH_VOLTAGE_WARNING		BIT(22)
+#define TPS_REG_INT_USB_HOST_PRESENT_NO_LONGER		BIT(21)
+#define TPS_REG_INT_USB_HOST_PRESENT			BIT(20)
+#define TPS_REG_INT_GOTO_MIN_RECEIVED			BIT(19)
+#define TPS_REG_INT_PR_SWAP_REQUESTED			BIT(17)
+#define TPS_REG_INT_SINK_CAP_MESSAGE_READY		BIT(15)
+#define TPS_REG_INT_SOURCE_CAP_MESSAGE_READY		BIT(14)
+#define TPS_REG_INT_NEW_CONTRACT_AS_PROVIDER		BIT(13)
+#define TPS_REG_INT_NEW_CONTRACT_AS_CONSUMER		BIT(12)
+#define TPS_REG_INT_VDM_RECEIVED			BIT(11)
+#define TPS_REG_INT_ATTENTION_RECEIVED			BIT(10)
+#define TPS_REG_INT_OVERCURRENT				BIT(9)
+#define TPS_REG_INT_BIST				BIT(8)
+#define TPS_REG_INT_RDO_RECEIVED_FROM_SINK		BIT(7)
+#define TPS_REG_INT_DR_SWAP_COMPLETE			BIT(5)
+#define TPS_REG_INT_PR_SWAP_COMPLETE			BIT(4)
+#define TPS_REG_INT_PLUG_EVENT				BIT(3)
+#define TPS_REG_INT_HARD_RESET				BIT(1)
+#define TPS_REG_INT_PD_SOFT_RESET			BIT(0)
+
+#endif /* __TPS6598X_H__ */
diff --git a/drivers/usb/typec/tps6598x_trace.h b/drivers/usb/typec/tps6598x_trace.h
new file mode 100644
index 000000000000..4ec96e3b2c3e
--- /dev/null
+++ b/drivers/usb/typec/tps6598x_trace.h
@@ -0,0 +1,97 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Driver for TI TPS6598x USB Power Delivery controller family
+ *
+ * Copyright (C) 2020 Purism SPC
+ * Author: Guido Günther <agx@sigxcpu.org>
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM tps6598x
+
+#if !defined(_TPS6598x_TRACE_H_) || defined(TRACE_HEADER_MULTI_READ)
+#define _TPS6598X_TRACE_H_
+
+#include "tps6598x.h"
+
+#include <linux/stringify.h>
+#include <linux/types.h>
+#include <linux/tracepoint.h>
+
+#define show_irq_flags(flags) \
+	__print_flags_u64(flags, "|", \
+		{ TPS_REG_INT_PD_SOFT_RESET,			"PD_SOFT_RESET" }, \
+		{ TPS_REG_INT_HARD_RESET,			"HARD_RESET" }, \
+		{ TPS_REG_INT_PLUG_EVENT,			"PLUG_EVENT" }, \
+		{ TPS_REG_INT_PR_SWAP_COMPLETE,			"PR_SWAP_COMPLETE" }, \
+		{ TPS_REG_INT_DR_SWAP_COMPLETE,			"DR_SWAP_COMPLETE" }, \
+		{ TPS_REG_INT_RDO_RECEIVED_FROM_SINK,		"RDO_RECEIVED_FROM_SINK" }, \
+		{ TPS_REG_INT_BIST,				"BIST" }, \
+		{ TPS_REG_INT_OVERCURRENT,			"OVERCURRENT" }, \
+		{ TPS_REG_INT_ATTENTION_RECEIVED,		"ATTENTION_RECEIVED" }, \
+		{ TPS_REG_INT_VDM_RECEIVED,			"VDM_RECEIVED" }, \
+		{ TPS_REG_INT_NEW_CONTRACT_AS_CONSUMER,		"NEW_CONTRACT_AS_CONSUMER" }, \
+		{ TPS_REG_INT_NEW_CONTRACT_AS_PROVIDER,		"NEW_CONTRACT_AS_PROVIDER" }, \
+		{ TPS_REG_INT_SOURCE_CAP_MESSAGE_READY,		"SOURCE_CAP_MESSAGE_READY" }, \
+		{ TPS_REG_INT_SINK_CAP_MESSAGE_READY,		"SINK_CAP_MESSAGE_READY" }, \
+		{ TPS_REG_INT_PR_SWAP_REQUESTED,		"PR_SWAP_REQUESTED" }, \
+		{ TPS_REG_INT_GOTO_MIN_RECEIVED,		"GOTO_MIN_RECEIVED" }, \
+		{ TPS_REG_INT_USB_HOST_PRESENT,			"USB_HOST_PRESENT" }, \
+		{ TPS_REG_INT_USB_HOST_PRESENT_NO_LONGER,	"USB_HOST_PRESENT_NO_LONGER" }, \
+		{ TPS_REG_INT_HIGH_VOLTAGE_WARNING,		"HIGH_VOLTAGE_WARNING" }, \
+		{ TPS_REG_INT_PP_SWITCH_CHANGED,		"PP_SWITCH_CHANGED" }, \
+		{ TPS_REG_INT_POWER_STATUS_UPDATE,		"POWER_STATUS_UPDATE" }, \
+		{ TPS_REG_INT_DATA_STATUS_UPDATE,		"DATA_STATUS_UPDATE" }, \
+		{ TPS_REG_INT_STATUS_UPDATE,			"STATUS_UPDATE" }, \
+		{ TPS_REG_INT_PD_STATUS_UPDATE,			"PD_STATUS_UPDATE" }, \
+		{ TPS_REG_INT_ADC_LOW_THRESHOLD,		"ADC_LOW_THRESHOLD" }, \
+		{ TPS_REG_INT_ADC_HIGH_THRESHOLD,		"ADC_HIGH_THRESHOLD" }, \
+		{ TPS_REG_INT_CMD1_COMPLETE,			"CMD1_COMPLETE" }, \
+		{ TPS_REG_INT_CMD2_COMPLETE,			"CMD2_COMPLETE" }, \
+		{ TPS_REG_INT_ERROR_DEVICE_INCOMPATIBLE,	"ERROR_DEVICE_INCOMPATIBLE" }, \
+		{ TPS_REG_INT_ERROR_CANNOT_PROVIDE_PWR,		"ERROR_CANNOT_PROVIDE_PWR" }, \
+		{ TPS_REG_INT_ERROR_CAN_PROVIDE_PWR_LATER,	"ERROR_CAN_PROVIDE_PWR_LATER" }, \
+		{ TPS_REG_INT_ERROR_POWER_EVENT_OCCURRED,	"ERROR_POWER_EVENT_OCCURRED" }, \
+		{ TPS_REG_INT_ERROR_MISSING_GET_CAP_MESSAGE,	"ERROR_MISSING_GET_CAP_MESSAGE" }, \
+		{ TPS_REG_INT_ERROR_PROTOCOL_ERROR,		"ERROR_PROTOCOL_ERROR" }, \
+		{ TPS_REG_INT_ERROR_MESSAGE_DATA,		"ERROR_MESSAGE_DATA" }, \
+		{ TPS_REG_INT_ERROR_DISCHARGE_FAILED,		"ERROR_DISCHARGE_FAILED" }, \
+		{ TPS_REG_INT_SRC_TRANSITION,			"SRC_TRANSITION" }, \
+		{ TPS_REG_INT_ERROR_UNABLE_TO_SOURCE,		"ERROR_UNABLE_TO_SOURCE" }, \
+		{ TPS_REG_INT_VDM_ENTERED_MODE,			"VDM_ENTERED_MODE" }, \
+		{ TPS_REG_INT_VDM_MSG_SENT,			"VDM_MSG_SENT" }, \
+		{ TPS_REG_INT_DISCOVER_MODES_COMPLETE,		"DISCOVER_MODES_COMPLETE" }, \
+		{ TPS_REG_INT_EXIT_MODES_COMPLETE,		"EXIT_MODES_COMPLETE" }, \
+		{ TPS_REG_INT_USER_VID_ALT_MODE_ENTERED,	"USER_VID_ALT_MODE_ENTERED" }, \
+		{ TPS_REG_INT_USER_VID_ALT_MODE_EXIT,		"USER_VID_ALT_MODE_EXIT" }, \
+		{ TPS_REG_INT_USER_VID_ALT_MODE_ATTN_VDM,	"USER_VID_ALT_MODE_ATTN_VDM" }, \
+		{ TPS_REG_INT_USER_VID_ALT_MODE_OTHER_VDM,	"USER_VID_ALT_MODE_OTHER_VDM" })
+
+TRACE_EVENT(tps6598x_irq,
+	    TP_PROTO(u64 event1,
+		     u64 event2),
+	    TP_ARGS(event1, event2),
+
+	    TP_STRUCT__entry(
+			     __field(u64, event1)
+			     __field(u64, event2)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->event1 = event1;
+			   __entry->event2 = event2;
+			   ),
+
+	    TP_printk("event1=%s, event2=%s",
+		      show_irq_flags(__entry->event1),
+		      show_irq_flags(__entry->event2))
+);
+
+#endif /* _TPS6598X_TRACE_H_ */
+
+/* This part must be outside protection */
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_FILE tps6598x_trace
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+#include <trace/define_trace.h>
-- 
2.30.0

[PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Guido Günther]

This allows to trace status information which helps to debug problems
with role switching, etc.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/usb/typec/tps6598x.c       | 26 ++++-----
 drivers/usb/typec/tps6598x.h       | 66 +++++++++++++++++++++
 drivers/usb/typec/tps6598x_trace.h | 94 ++++++++++++++++++++++++++++++
 3 files changed, 171 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index bc34b35e909f..559aa175f948 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -37,13 +37,6 @@
 #define TPS_REG_POWER_STATUS		0x3f
 #define TPS_REG_RX_IDENTITY_SOP		0x48
 
-/* TPS_REG_STATUS bits */
-#define TPS_STATUS_PLUG_PRESENT		BIT(0)
-#define TPS_STATUS_ORIENTATION		BIT(4)
-#define TPS_STATUS_PORTROLE(s)		(!!((s) & BIT(5)))
-#define TPS_STATUS_DATAROLE(s)		(!!((s) & BIT(6)))
-#define TPS_STATUS_VCONN(s)		(!!((s) & BIT(7)))
-
 /* TPS_REG_SYSTEM_CONF bits */
 #define TPS_SYSCONF_PORTINFO(c)		((c) & 7)
 
@@ -258,9 +251,9 @@ static int tps6598x_connect(struct tps6598x *tps, u32 status)
 	}
 
 	typec_set_pwr_opmode(tps->port, mode);
-	typec_set_pwr_role(tps->port, TPS_STATUS_PORTROLE(status));
-	typec_set_vconn_role(tps->port, TPS_STATUS_VCONN(status));
-	tps6598x_set_data_role(tps, TPS_STATUS_DATAROLE(status), true);
+	typec_set_pwr_role(tps->port, TPS_STATUS_TO_TYPEC_PORTROLE(status));
+	typec_set_vconn_role(tps->port, TPS_STATUS_TO_TYPEC_VCONN(status));
+	tps6598x_set_data_role(tps, TPS_STATUS_TO_TYPEC_DATAROLE(status), true);
 
 	tps->partner = typec_register_partner(tps->port, &desc);
 	if (IS_ERR(tps->partner))
@@ -280,9 +273,10 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
 		typec_unregister_partner(tps->partner);
 	tps->partner = NULL;
 	typec_set_pwr_opmode(tps->port, TYPEC_PWR_MODE_USB);
-	typec_set_pwr_role(tps->port, TPS_STATUS_PORTROLE(status));
-	typec_set_vconn_role(tps->port, TPS_STATUS_VCONN(status));
-	tps6598x_set_data_role(tps, TPS_STATUS_DATAROLE(status), false);
+	typec_set_pwr_role(tps->port, TPS_STATUS_TO_TYPEC_PORTROLE(status));
+	typec_set_vconn_role(tps->port, TPS_STATUS_TO_TYPEC_VCONN(status));
+	tps6598x_set_data_role(tps, TPS_STATUS_TO_TYPEC_DATAROLE(status), false);
+
 	power_supply_changed(tps->psy);
 }
 
@@ -366,7 +360,7 @@ static int tps6598x_dr_set(struct typec_port *port, enum typec_data_role role)
 	if (ret)
 		goto out_unlock;
 
-	if (role != TPS_STATUS_DATAROLE(status)) {
+	if (role != TPS_STATUS_TO_TYPEC_DATAROLE(status)) {
 		ret = -EPROTO;
 		goto out_unlock;
 	}
@@ -396,7 +390,7 @@ static int tps6598x_pr_set(struct typec_port *port, enum typec_role role)
 	if (ret)
 		goto out_unlock;
 
-	if (role != TPS_STATUS_PORTROLE(status)) {
+	if (role != TPS_STATUS_TO_TYPEC_PORTROLE(status)) {
 		ret = -EPROTO;
 		goto out_unlock;
 	}
@@ -437,6 +431,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 		dev_err(tps->dev, "%s: failed to read status\n", __func__);
 		goto err_clear_ints;
 	}
+	trace_tps6598x_status(status);
 
 	/* Handle plug insert or removal */
 	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT) {
@@ -612,6 +607,7 @@ static int tps6598x_probe(struct i2c_client *client)
 	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
 	if (ret < 0)
 		return ret;
+	trace_tps6598x_status(status);
 
 	ret = tps6598x_read32(tps, TPS_REG_SYSTEM_CONF, &conf);
 	if (ret < 0)
diff --git a/drivers/usb/typec/tps6598x.h b/drivers/usb/typec/tps6598x.h
index b83b8a6a1504..866fd1deb471 100644
--- a/drivers/usb/typec/tps6598x.h
+++ b/drivers/usb/typec/tps6598x.h
@@ -12,6 +12,72 @@
 #ifndef __TPS6598X_H__
 #define __TPS6598X_H__
 
+/* TPS_REG_STATUS bits */
+#define TPS_STATUS_PLUG_PRESENT		BIT(0)
+#define TPS_STATUS_PLUG_UPSIDE_DOWN	BIT(4)
+#define TPS_STATUS_PORTROLE		BIT(5)
+#define TPS_STATUS_TO_TYPEC_PORTROLE(s) (!!((s) & TPS_STATUS_PORTROLE))
+#define TPS_STATUS_DATAROLE		BIT(6)
+#define TPS_STATUS_TO_TYPEC_DATAROLE(s)	(!!((s) & TPS_STATUS_DATAROLE))
+#define TPS_STATUS_VCONN		BIT(7)
+#define TPS_STATUS_TO_TYPEC_VCONN(s)	(!!((s) & TPS_STATUS_VCONN))
+#define TPS_STATUS_OVERCURRENT		BIT(16)
+#define TPS_STATUS_GOTO_MIN_ACTIVE	BIT(26)
+#define TPS_STATUS_BIST			BIT(27)
+#define TPS_STATUS_HIGH_VOLAGE_WARNING	BIT(28)
+#define TPS_STATUS_HIGH_LOW_VOLTAGE_WARNING BIT(29)
+
+#define TPS_STATUS_CONN_STATE_MASK		GENMASK(3, 1)
+#define TPS_STATUS_CONN_STATE(x)		FIELD_GET(TPS_STATUS_CONN_STATE_MASK, (x))
+#define TPS_STATUS_PP_5V0_SWITCH_MASK		GENMASK(9, 8)
+#define TPS_STATUS_PP_5V0_SWITCH(x)		FIELD_GET(TPS_STATUS_PP_5V0_SWITCH_MASK, (x))
+#define TPS_STATUS_PP_HV_SWITCH_MASK		GENMASK(11, 10)
+#define TPS_STATUS_PP_HV_SWITCH(x)		FIELD_GET(TPS_STATUS_PP_HV_SWITCH_MASK, (x))
+#define TPS_STATUS_PP_EXT_SWITCH_MASK		GENMASK(13, 12)
+#define TPS_STATUS_PP_EXT_SWITCH(x)		FIELD_GET(TPS_STATUS_PP_EXT_SWITCH_MASK, (x))
+#define TPS_STATUS_PP_CABLE_SWITCH_MASK		GENMASK(15, 14)
+#define TPS_STATUS_PP_CABLE_SWITCH(x)		FIELD_GET(TPS_STATUS_PP_CABLE_SWITCH_MASK, (x))
+#define TPS_STATUS_POWER_SOURCE_MASK		GENMASK(19, 18)
+#define TPS_STATUS_POWER_SOURCE(x)		FIELD_GET(TPS_STATUS_POWER_SOURCE_MASK, (x))
+#define TPS_STATUS_VBUS_STATUS_MASK		GENMASK(21, 20)
+#define TPS_STATUS_VBUS_STATUS(x)		FIELD_GET(TPS_STATUS_VBUS_STATUS_MASK, (x))
+#define TPS_STATUS_USB_HOST_PRESENT_MASK	GENMASK(23, 22)
+#define TPS_STATUS_USB_HOST_PRESENT(x)		FIELD_GET(TPS_STATUS_USB_HOST_PRESENT_MASK, (x))
+#define TPS_STATUS_LEGACY_MASK			GENMASK(25, 24)
+#define TPS_STATUS_LEGACY(x)			FIELD_GET(TPS_STATUS_LEGACY_MASK, (x))
+
+#define TPS_STATUS_CONN_STATE_NO_CONN		0
+#define TPS_STATUS_CONN_STATE_DISABLED		1
+#define TPS_STATUS_CONN_STATE_AUDIO_CONN	2
+#define TPS_STATUS_CONN_STATE_DEBUG_CONN	3
+#define TPS_STATUS_CONN_STATE_NO_CONN_R_A	4
+#define TPS_STATUS_CONN_STATE_RESERVED		5
+#define TPS_STATUS_CONN_STATE_CONN_NO_R_A	6
+#define TPS_STATUS_CONN_STATE_CONN_WITH_R_A	7
+
+#define TPS_STATUS_PP_SWITCH_STATE_DISABLED	0
+#define TPS_STATUS_PP_SWITCH_STATE_FAULT	1
+#define TPS_STATUS_PP_SWITCH_STATE_OUT		2
+#define TPS_STATUS_PP_SWITCH_STATE_IN		3
+
+#define TPS_STATUS_POWER_SOURCE_UNKNOWN		0
+#define TPS_STATUS_POWER_SOURCE_VIN_3P3		1
+#define TPS_STATUS_POWER_SOURCE_DEAD_BAT	2
+#define TPS_STATUS_POWER_SOURCE_VBUS		3
+
+#define TPS_STATUS_VBUS_STATUS_VSAFE0V		0
+#define TPS_STATUS_VBUS_STATUS_VSAFE5V		1
+#define TPS_STATUS_VBUS_STATUS_PD		2
+#define TPS_STATUS_VBUS_STATUS_FAULT		3
+
+#define TPS_STATUS_USB_HOST_PRESENT_NO		0
+#define TPS_STATUS_USB_HOST_PRESENT_PD_NO_USB	1
+#define TPS_STATUS_USB_HOST_PRESENT_NO_PD	2
+#define TPS_STATUS_USB_HOST_PRESENT_PD_USB	3
+
+#define TPS_STATUS_LEGACY_NO			0
+#define TPS_STATUS_LEGACY_SINK			1
+#define TPS_STATUS_LEGACY_SOURCE		2
 
 /* TPS_REG_INT_* bits */
 #define TPS_REG_INT_USER_VID_ALT_MODE_OTHER_VDM		BIT_ULL(27+32)
diff --git a/drivers/usb/typec/tps6598x_trace.h b/drivers/usb/typec/tps6598x_trace.h
index 4ec96e3b2c3e..e0677b9c5c53 100644
--- a/drivers/usb/typec/tps6598x_trace.h
+++ b/drivers/usb/typec/tps6598x_trace.h
@@ -67,6 +67,73 @@
 		{ TPS_REG_INT_USER_VID_ALT_MODE_ATTN_VDM,	"USER_VID_ALT_MODE_ATTN_VDM" }, \
 		{ TPS_REG_INT_USER_VID_ALT_MODE_OTHER_VDM,	"USER_VID_ALT_MODE_OTHER_VDM" })
 
+#define TPS6598X_STATUS_FLAGS_MASK (GENMASK(31, 0) ^ (TPS_STATUS_CONN_STATE_MASK | \
+						      TPS_STATUS_PP_5V0_SWITCH_MASK | \
+						      TPS_STATUS_PP_HV_SWITCH_MASK | \
+						      TPS_STATUS_PP_EXT_SWITCH_MASK | \
+						      TPS_STATUS_PP_CABLE_SWITCH_MASK | \
+						      TPS_STATUS_POWER_SOURCE_MASK | \
+						      TPS_STATUS_VBUS_STATUS_MASK | \
+						      TPS_STATUS_USB_HOST_PRESENT_MASK | \
+						      TPS_STATUS_LEGACY_MASK))
+
+#define show_status_conn_state(status) \
+	__print_symbolic(TPS_STATUS_CONN_STATE((status)), \
+		{ TPS_STATUS_CONN_STATE_CONN_WITH_R_A,	"conn-Ra"  }, \
+		{ TPS_STATUS_CONN_STATE_CONN_NO_R_A,	"conn-no-Ra" }, \
+		{ TPS_STATUS_CONN_STATE_NO_CONN_R_A,	"no-conn-Ra" },	\
+		{ TPS_STATUS_CONN_STATE_DEBUG_CONN,	"debug"	 }, \
+		{ TPS_STATUS_CONN_STATE_AUDIO_CONN,	"audio"	 }, \
+		{ TPS_STATUS_CONN_STATE_DISABLED,	"disabled" }, \
+		{ TPS_STATUS_CONN_STATE_NO_CONN,	"no-conn" })
+
+#define show_status_pp_switch_state(status) \
+	__print_symbolic(status, \
+		{ TPS_STATUS_PP_SWITCH_STATE_IN,	"in" }, \
+		{ TPS_STATUS_PP_SWITCH_STATE_OUT,	"out" }, \
+		{ TPS_STATUS_PP_SWITCH_STATE_FAULT,	"fault" }, \
+		{ TPS_STATUS_PP_SWITCH_STATE_DISABLED,	"off" })
+
+#define show_status_power_sources(status) \
+	__print_symbolic(TPS_STATUS_POWER_SOURCE(status), \
+		{ TPS_STATUS_POWER_SOURCE_VBUS,		"vbus" }, \
+		{ TPS_STATUS_POWER_SOURCE_VIN_3P3,	"vin-3p3" }, \
+		{ TPS_STATUS_POWER_SOURCE_DEAD_BAT,	"dead-battery" }, \
+		{ TPS_STATUS_POWER_SOURCE_UNKNOWN,	"unknown" })
+
+#define show_status_vbus_status(status) \
+	__print_symbolic(TPS_STATUS_VBUS_STATUS(status), \
+		{ TPS_STATUS_VBUS_STATUS_VSAFE0V,	"vSafe0V" }, \
+		{ TPS_STATUS_VBUS_STATUS_VSAFE5V,	"vSafe5V" }, \
+		{ TPS_STATUS_VBUS_STATUS_PD,		"pd" }, \
+		{ TPS_STATUS_VBUS_STATUS_FAULT,		"fault" })
+
+#define show_status_usb_host_present(status) \
+	__print_symbolic(TPS_STATUS_USB_HOST_PRESENT(status), \
+		{ TPS_STATUS_USB_HOST_PRESENT_PD_USB,	 "pd-usb" }, \
+		{ TPS_STATUS_USB_HOST_PRESENT_NO_PD,	 "no-pd" }, \
+		{ TPS_STATUS_USB_HOST_PRESENT_PD_NO_USB, "pd-no-usb" }, \
+		{ TPS_STATUS_USB_HOST_PRESENT_NO,	 "no" })
+
+#define show_status_legacy(status) \
+	__print_symbolic(TPS_STATUS_LEGACY(status),	     \
+		{ TPS_STATUS_LEGACY_SOURCE,		 "source" }, \
+		{ TPS_STATUS_LEGACY_SINK,		 "sink" }, \
+		{ TPS_STATUS_LEGACY_NO,			 "no" })
+
+#define show_status_flags(flags) \
+	__print_flags((flags & TPS6598X_STATUS_FLAGS_MASK), "|", \
+		      { TPS_STATUS_PLUG_PRESENT,	"PLUG_PRESENT" }, \
+		      { TPS_STATUS_PLUG_UPSIDE_DOWN,	"UPSIDE_DOWN" }, \
+		      { TPS_STATUS_PORTROLE,		"PORTROLE" }, \
+		      { TPS_STATUS_DATAROLE,		"DATAROLE" }, \
+		      { TPS_STATUS_VCONN,		"VCONN" }, \
+		      { TPS_STATUS_OVERCURRENT,		"OVERCURRENT" }, \
+		      { TPS_STATUS_GOTO_MIN_ACTIVE,	"GOTO_MIN_ACTIVE" }, \
+		      { TPS_STATUS_BIST,		"BIST" }, \
+		      { TPS_STATUS_HIGH_VOLAGE_WARNING,	"HIGH_VOLAGE_WARNING" }, \
+		      { TPS_STATUS_HIGH_LOW_VOLTAGE_WARNING, "HIGH_LOW_VOLTAGE_WARNING" })
+
 TRACE_EVENT(tps6598x_irq,
 	    TP_PROTO(u64 event1,
 		     u64 event2),
@@ -87,6 +154,33 @@ TRACE_EVENT(tps6598x_irq,
 		      show_irq_flags(__entry->event2))
 );
 
+TRACE_EVENT(tps6598x_status,
+	    TP_PROTO(u32 status),
+	    TP_ARGS(status),
+
+	    TP_STRUCT__entry(
+			     __field(u32, status)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->status = status;
+			   ),
+
+	    TP_printk("conn: %s, pp_5v0: %s, pp_hv: %s, pp_ext: %s, pp_cable: %s, "
+		      "pwr-src: %s, vbus: %s, usb-host: %s, legacy: %s, flags: %s",
+		      show_status_conn_state(__entry->status),
+		      show_status_pp_switch_state(TPS_STATUS_PP_5V0_SWITCH(__entry->status)),
+		      show_status_pp_switch_state(TPS_STATUS_PP_HV_SWITCH(__entry->status)),
+		      show_status_pp_switch_state(TPS_STATUS_PP_EXT_SWITCH(__entry->status)),
+		      show_status_pp_switch_state(TPS_STATUS_PP_CABLE_SWITCH(__entry->status)),
+		      show_status_power_sources(__entry->status),
+		      show_status_vbus_status(__entry->status),
+		      show_status_usb_host_present(__entry->status),
+		      show_status_legacy(__entry->status),
+		      show_status_flags(__entry->status)
+		    )
+);
+
 #endif /* _TPS6598X_TRACE_H_ */
 
 /* This part must be outside protection */
-- 
2.30.0

[PATCH v2 3/4] usb: typec: tps6598x: Add trace event for power status register

[Guido Günther]

Together with the PD status register this is vital for debugging power
negotiations at runtime.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/usb/typec/tps6598x.c       | 19 +++++++++------
 drivers/usb/typec/tps6598x.h       | 19 +++++++++++++++
 drivers/usb/typec/tps6598x_trace.h | 38 ++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index 559aa175f948..3e6ad3ba7fc8 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -50,11 +50,6 @@ enum {
 	TPS_PORTINFO_SOURCE,
 };
 
-/* TPS_REG_POWER_STATUS bits */
-#define TPS_POWER_STATUS_CONNECTION	BIT(0)
-#define TPS_POWER_STATUS_SOURCESINK	BIT(1)
-#define TPS_POWER_STATUS_PWROPMODE(p)	(((p) & GENMASK(3, 2)) >> 2)
-
 /* TPS_REG_RX_IDENTITY_SOP */
 struct tps6598x_rx_identity_reg {
 	u8 status;
@@ -414,6 +409,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 	u64 event1;
 	u64 event2;
 	u32 status;
+	u16 pwr_status;
 	int ret;
 
 	mutex_lock(&tps->lock);
@@ -433,6 +429,15 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 	}
 	trace_tps6598x_status(status);
 
+	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE) {
+		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
+		if (ret < 0) {
+			dev_err(tps->dev, "failed to read power status: %d\n", ret);
+			goto err_clear_ints;
+		}
+		trace_tps6598x_power_status(pwr_status);
+	}
+
 	/* Handle plug insert or removal */
 	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT) {
 		if (status & TPS_STATUS_PLUG_PRESENT) {
@@ -497,8 +502,8 @@ static int tps6598x_psy_get_online(struct tps6598x *tps,
 	if (ret < 0)
 		return ret;
 
-	if ((pwr_status & TPS_POWER_STATUS_CONNECTION) &&
-	    (pwr_status & TPS_POWER_STATUS_SOURCESINK)) {
+	if (TPS_POWER_STATUS_CONNECTION(pwr_status) &&
+	    TPS_POWER_STATUS_SOURCESINK(pwr_status)) {
 		val->intval = 1;
 	} else {
 		val->intval = 0;
diff --git a/drivers/usb/typec/tps6598x.h b/drivers/usb/typec/tps6598x.h
index 866fd1deb471..3f6503377678 100644
--- a/drivers/usb/typec/tps6598x.h
+++ b/drivers/usb/typec/tps6598x.h
@@ -127,4 +127,23 @@
 #define TPS_REG_INT_HARD_RESET				BIT(1)
 #define TPS_REG_INT_PD_SOFT_RESET			BIT(0)
 
+/* TPS_REG_POWER_STATUS bits */
+#define TPS_POWER_STATUS_CONNECTION(x)  FIELD_GET(BIT(0), (x))
+#define TPS_POWER_STATUS_SOURCESINK(x)	FIELD_GET(BIT(1), (x))
+#define TPS_POWER_STATUS_BC12_DET(x)	FIELD_GET(BIT(2), (x))
+
+#define TPS_POWER_STATUS_TYPEC_CURRENT_MASK GENMASK(3, 2)
+#define TPS_POWER_STATUS_PWROPMODE(p)	    FIELD_GET(TPS_POWER_STATUS_TYPEC_CURRENT_MASK, (p))
+#define TPS_POWER_STATUS_BC12_STATUS_MASK   GENMASK(6, 5)
+#define TPS_POWER_STATUS_BC12_STATUS(p)	    FIELD_GET(TPS_POWER_STATUS_BC12_STATUS_MASK, (p))
+
+#define TPS_POWER_STATUS_TYPEC_CURRENT_USB     0
+#define TPS_POWER_STATUS_TYPEC_CURRENT_1A5     1
+#define TPS_POWER_STATUS_TYPEC_CURRENT_3A0     2
+#define TPS_POWER_STATUS_TYPEC_CURRENT_PD      3
+
+#define TPS_POWER_STATUS_BC12_STATUS_SDP 0
+#define TPS_POWER_STATUS_BC12_STATUS_CDP 2
+#define TPS_POWER_STATUS_BC12_STATUS_DCP 3
+
 #endif /* __TPS6598X_H__ */
diff --git a/drivers/usb/typec/tps6598x_trace.h b/drivers/usb/typec/tps6598x_trace.h
index e0677b9c5c53..78a5a6ca337b 100644
--- a/drivers/usb/typec/tps6598x_trace.h
+++ b/drivers/usb/typec/tps6598x_trace.h
@@ -134,6 +134,24 @@
 		      { TPS_STATUS_HIGH_VOLAGE_WARNING,	"HIGH_VOLAGE_WARNING" }, \
 		      { TPS_STATUS_HIGH_LOW_VOLTAGE_WARNING, "HIGH_LOW_VOLTAGE_WARNING" })
 
+#define show_power_status_source_sink(power_status) \
+	__print_symbolic(TPS_POWER_STATUS_SOURCESINK(power_status), \
+		{ 1, "sink" }, \
+		{ 0, "source" })
+
+#define show_power_status_typec_status(power_status) \
+	__print_symbolic(TPS_POWER_STATUS_PWROPMODE(power_status), \
+		{ TPS_POWER_STATUS_TYPEC_CURRENT_PD,  "pd" }, \
+		{ TPS_POWER_STATUS_TYPEC_CURRENT_3A0, "3.0A" }, \
+		{ TPS_POWER_STATUS_TYPEC_CURRENT_1A5, "1.5A" }, \
+		{ TPS_POWER_STATUS_TYPEC_CURRENT_USB, "usb" })
+
+#define show_power_status_bc12_status(power_status) \
+	__print_symbolic(TPS_POWER_STATUS_BC12_STATUS(power_status), \
+		{ TPS_POWER_STATUS_BC12_STATUS_DCP, "dcp" }, \
+		{ TPS_POWER_STATUS_BC12_STATUS_CDP, "cdp" }, \
+		{ TPS_POWER_STATUS_BC12_STATUS_SDP, "sdp" })
+
 TRACE_EVENT(tps6598x_irq,
 	    TP_PROTO(u64 event1,
 		     u64 event2),
@@ -181,6 +199,26 @@ TRACE_EVENT(tps6598x_status,
 		    )
 );
 
+TRACE_EVENT(tps6598x_power_status,
+	    TP_PROTO(u16 power_status),
+	    TP_ARGS(power_status),
+
+	    TP_STRUCT__entry(
+			     __field(u16, power_status)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->power_status = power_status;
+			   ),
+
+	    TP_printk("conn: %d, pwr-role: %s, typec: %s, bc: %s",
+		      !!TPS_POWER_STATUS_CONNECTION(__entry->power_status),
+		      show_power_status_source_sink(__entry->power_status),
+		      show_power_status_typec_status(__entry->power_status),
+		      show_power_status_bc12_status(__entry->power_status)
+		    )
+);
+
 #endif /* _TPS6598X_TRACE_H_ */
 
 /* This part must be outside protection */
-- 
2.30.0

[PATCH v2 4/4] usb: typec: tps6598x: Add trace event for data status

[Guido Günther]

This is useful to debug DP negotiation and pin assignment even
when the firmware does all the work.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/usb/typec/tps6598x.c       | 12 ++++++-
 drivers/usb/typec/tps6598x.h       | 36 ++++++++++++++++++++
 drivers/usb/typec/tps6598x_trace.h | 54 ++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index 3e6ad3ba7fc8..a4ec8e56c2b9 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -36,6 +36,7 @@
 #define TPS_REG_CTRL_CONF		0x29
 #define TPS_REG_POWER_STATUS		0x3f
 #define TPS_REG_RX_IDENTITY_SOP		0x48
+#define TPS_REG_DATA_STATUS		0x5f
 
 /* TPS_REG_SYSTEM_CONF bits */
 #define TPS_SYSCONF_PORTINFO(c)		((c) & 7)
@@ -408,7 +409,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 	struct tps6598x *tps = data;
 	u64 event1;
 	u64 event2;
-	u32 status;
+	u32 status, data_status;
 	u16 pwr_status;
 	int ret;
 
@@ -438,6 +439,15 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 		trace_tps6598x_power_status(pwr_status);
 	}
 
+	if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE) {
+		ret = tps6598x_read32(tps, TPS_REG_DATA_STATUS, &data_status);
+		if (ret < 0) {
+			dev_err(tps->dev, "failed to read data status: %d\n", ret);
+			goto err_clear_ints;
+		}
+		trace_tps6598x_data_status(data_status);
+	}
+
 	/* Handle plug insert or removal */
 	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT) {
 		if (status & TPS_STATUS_PLUG_PRESENT) {
diff --git a/drivers/usb/typec/tps6598x.h b/drivers/usb/typec/tps6598x.h
index 3f6503377678..1afc22ab4dbb 100644
--- a/drivers/usb/typec/tps6598x.h
+++ b/drivers/usb/typec/tps6598x.h
@@ -146,4 +146,40 @@
 #define TPS_POWER_STATUS_BC12_STATUS_CDP 2
 #define TPS_POWER_STATUS_BC12_STATUS_DCP 3
 
+/* TPS_REG_DATA_STATUS bits */
+#define TPS_DATA_STATUS_DATA_CONNECTION	     BIT(0)
+#define TPS_DATA_STATUS_UPSIDE_DOWN	     BIT(1)
+#define TPS_DATA_STATUS_ACTIVE_CABLE	     BIT(2)
+#define TPS_DATA_STATUS_USB2_CONNECTION	     BIT(4)
+#define TPS_DATA_STATUS_USB3_CONNECTION	     BIT(5)
+#define TPS_DATA_STATUS_USB3_GEN2	     BIT(6)
+#define TPS_DATA_STATUS_USB_DATA_ROLE	     BIT(7)
+#define TPS_DATA_STATUS_DP_CONNECTION	     BIT(8)
+#define TPS_DATA_STATUS_DP_SINK		     BIT(9)
+#define TPS_DATA_STATUS_TBT_CONNECTION	     BIT(16)
+#define TPS_DATA_STATUS_TBT_TYPE	     BIT(17)
+#define TPS_DATA_STATUS_OPTICAL_CABLE	     BIT(18)
+#define TPS_DATA_STATUS_ACTIVE_LINK_TRAIN    BIT(20)
+#define TPS_DATA_STATUS_FORCE_LSX	     BIT(23)
+#define TPS_DATA_STATUS_POWER_MISMATCH	     BIT(24)
+
+#define TPS_DATA_STATUS_DP_PIN_ASSIGNMENT_MASK GENMASK(11, 10)
+#define TPS_DATA_STATUS_DP_PIN_ASSIGNMENT(x) \
+	FIELD_GET(TPS_DATA_STATUS_DP_PIN_ASSIGNMENT_MASK, (x))
+#define TPS_DATA_STATUS_TBT_CABLE_SPEED_MASK   GENMASK(27, 25)
+#define TPS_DATA_STATUS_TBT_CABLE_SPEED	       FIELD_GET(TPS_DATA_STATUS_TBT_CABLE_SPEED_MASK, (x))
+#define TPS_DATA_STATUS_TBT_CABLE_GEN_MASK     GENMASK(29, 28)
+#define TPS_DATA_STATUS_TBT_CABLE_GEN	       FIELD_GET(TPS_DATA_STATUS_TBT_CABLE_GEN_MASK, (x))
+
+/* Map data status to DP spec assignments */
+#define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT(x) \
+	((TPS_DATA_STATUS_DP_PIN_ASSIGNMENT(x) << 1) | \
+		FIELD_GET(TPS_DATA_STATUS_USB3_CONNECTION, (x)))
+#define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_E    0
+#define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_F    BIT(0)
+#define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_C    BIT(1)
+#define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_D    (BIT(1) | BIT(0))
+#define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_A    BIT(2)
+#define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_B    (BIT(2) | BIT(1))
+
 #endif /* __TPS6598X_H__ */
diff --git a/drivers/usb/typec/tps6598x_trace.h b/drivers/usb/typec/tps6598x_trace.h
index 78a5a6ca337b..38bfb2f04e46 100644
--- a/drivers/usb/typec/tps6598x_trace.h
+++ b/drivers/usb/typec/tps6598x_trace.h
@@ -152,6 +152,41 @@
 		{ TPS_POWER_STATUS_BC12_STATUS_CDP, "cdp" }, \
 		{ TPS_POWER_STATUS_BC12_STATUS_SDP, "sdp" })
 
+#define TPS_DATA_STATUS_FLAGS_MASK (GENMASK(31, 0) ^ (TPS_DATA_STATUS_DP_PIN_ASSIGNMENT_MASK | \
+						      TPS_DATA_STATUS_TBT_CABLE_SPEED_MASK | \
+						      TPS_DATA_STATUS_TBT_CABLE_GEN_MASK))
+
+#define show_data_status_flags(data_status) \
+	__print_flags(data_status & TPS_DATA_STATUS_FLAGS_MASK, "|", \
+		{ TPS_DATA_STATUS_DATA_CONNECTION,	"DATA_CONNECTION" }, \
+		{ TPS_DATA_STATUS_UPSIDE_DOWN,		"DATA_UPSIDE_DOWN" }, \
+		{ TPS_DATA_STATUS_ACTIVE_CABLE,		"ACTIVE_CABLE" }, \
+		{ TPS_DATA_STATUS_USB2_CONNECTION,	"USB2_CONNECTION" }, \
+		{ TPS_DATA_STATUS_USB3_CONNECTION,	"USB3_CONNECTION" }, \
+		{ TPS_DATA_STATUS_USB3_GEN2,		"USB3_GEN2" }, \
+		{ TPS_DATA_STATUS_USB_DATA_ROLE,	"USB_DATA_ROLE" }, \
+		{ TPS_DATA_STATUS_DP_CONNECTION,	"DP_CONNECTION" }, \
+		{ TPS_DATA_STATUS_DP_SINK,		"DP_SINK" }, \
+		{ TPS_DATA_STATUS_TBT_CONNECTION,	"TBT_CONNECTION" }, \
+		{ TPS_DATA_STATUS_TBT_TYPE,		"TBT_TYPE" }, \
+		{ TPS_DATA_STATUS_OPTICAL_CABLE,	"OPTICAL_CABLE" }, \
+		{ TPS_DATA_STATUS_ACTIVE_LINK_TRAIN,	"ACTIVE_LINK_TRAIN" }, \
+		{ TPS_DATA_STATUS_FORCE_LSX,		"FORCE_LSX" }, \
+		{ TPS_DATA_STATUS_POWER_MISMATCH,	"POWER_MISMATCH" })
+
+#define show_data_status_dp_pin_assignment(data_status) \
+	__print_symbolic(TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT(data_status), \
+		{ TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_E, "E" }, \
+		{ TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_F, "F" }, \
+		{ TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_C, "C" }, \
+		{ TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_D, "D" }, \
+		{ TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_A, "A" }, \
+		{ TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_B, "B" })
+
+#define maybe_show_data_status_dp_pin_assignment(data_status) \
+	(data_status & TPS_DATA_STATUS_DP_CONNECTION ? \
+	 show_data_status_dp_pin_assignment(data_status) : "")
+
 TRACE_EVENT(tps6598x_irq,
 	    TP_PROTO(u64 event1,
 		     u64 event2),
@@ -219,6 +254,25 @@ TRACE_EVENT(tps6598x_power_status,
 		    )
 );
 
+TRACE_EVENT(tps6598x_data_status,
+	    TP_PROTO(u32 data_status),
+	    TP_ARGS(data_status),
+
+	    TP_STRUCT__entry(
+			     __field(u32, data_status)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->data_status = data_status;
+			   ),
+
+	    TP_printk("%s%s%s",
+		      show_data_status_flags(__entry->data_status),
+		      __entry->data_status & TPS_DATA_STATUS_DP_CONNECTION ? ", DP pinout " : "",
+		      maybe_show_data_status_dp_pin_assignment(__entry->data_status)
+		    )
+);
+
 #endif /* _TPS6598X_TRACE_H_ */
 
 /* This part must be outside protection */
-- 
2.30.0

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4310 bytes --]

Hi "Guido,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on v5.11-rc7 next-20210211]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Guido-G-nther/usb-typec-tps6598x-Add-IRQ-flag-and-register-tracing/20210212-200855
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: openrisc-randconfig-s032-20210209 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-215-g0fb77bb6-dirty
        # https://github.com/0day-ci/linux/commit/ba45e1d5e1fd25b6aed8724106e6c7d5adef7a20
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Guido-G-nther/usb-typec-tps6598x-Add-IRQ-flag-and-register-tracing/20210212-200855
        git checkout ba45e1d5e1fd25b6aed8724106e6c7d5adef7a20
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=openrisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <rong.a.chen@intel.com>


"sparse warnings: (new ones prefixed by >>)"
   drivers/usb/typec/tps6598x.c: note: in included file (through include/trace/trace_events.h, include/trace/define_trace.h, drivers/usb/typec/tps6598x_trace.h):
>> drivers/usb/typec/./tps6598x_trace.h:157:1: sparse: sparse: too long token expansion

vim +157 drivers/usb/typec/./tps6598x_trace.h

c90c0282e4ce33 Guido Günther 2021-02-12  156  
ba45e1d5e1fd25 Guido Günther 2021-02-12 @157  TRACE_EVENT(tps6598x_status,
ba45e1d5e1fd25 Guido Günther 2021-02-12  158  	    TP_PROTO(u32 status),
ba45e1d5e1fd25 Guido Günther 2021-02-12  159  	    TP_ARGS(status),
ba45e1d5e1fd25 Guido Günther 2021-02-12  160  
ba45e1d5e1fd25 Guido Günther 2021-02-12  161  	    TP_STRUCT__entry(
ba45e1d5e1fd25 Guido Günther 2021-02-12  162  			     __field(u32, status)
ba45e1d5e1fd25 Guido Günther 2021-02-12  163  			     ),
ba45e1d5e1fd25 Guido Günther 2021-02-12  164  
ba45e1d5e1fd25 Guido Günther 2021-02-12  165  	    TP_fast_assign(
ba45e1d5e1fd25 Guido Günther 2021-02-12  166  			   __entry->status = status;
ba45e1d5e1fd25 Guido Günther 2021-02-12  167  			   ),
ba45e1d5e1fd25 Guido Günther 2021-02-12  168  
ba45e1d5e1fd25 Guido Günther 2021-02-12  169  	    TP_printk("conn: %s, pp_5v0: %s, pp_hv: %s, pp_ext: %s, pp_cable: %s, "
ba45e1d5e1fd25 Guido Günther 2021-02-12  170  		      "pwr-src: %s, vbus: %s, usb-host: %s, legacy: %s, flags: %s",
ba45e1d5e1fd25 Guido Günther 2021-02-12  171  		      show_status_conn_state(__entry->status),
ba45e1d5e1fd25 Guido Günther 2021-02-12  172  		      show_status_pp_switch_state(TPS_STATUS_PP_5V0_SWITCH(__entry->status)),
ba45e1d5e1fd25 Guido Günther 2021-02-12  173  		      show_status_pp_switch_state(TPS_STATUS_PP_HV_SWITCH(__entry->status)),
ba45e1d5e1fd25 Guido Günther 2021-02-12  174  		      show_status_pp_switch_state(TPS_STATUS_PP_EXT_SWITCH(__entry->status)),
ba45e1d5e1fd25 Guido Günther 2021-02-12  175  		      show_status_pp_switch_state(TPS_STATUS_PP_CABLE_SWITCH(__entry->status)),
ba45e1d5e1fd25 Guido Günther 2021-02-12  176  		      show_status_power_sources(__entry->status),
ba45e1d5e1fd25 Guido Günther 2021-02-12  177  		      show_status_vbus_status(__entry->status),
ba45e1d5e1fd25 Guido Günther 2021-02-12  178  		      show_status_usb_host_present(__entry->status),
ba45e1d5e1fd25 Guido Günther 2021-02-12  179  		      show_status_legacy(__entry->status),
ba45e1d5e1fd25 Guido Günther 2021-02-12  180  		      show_status_flags(__entry->status)
ba45e1d5e1fd25 Guido Günther 2021-02-12  181  		    )
ba45e1d5e1fd25 Guido Günther 2021-02-12  182  );
ba45e1d5e1fd25 Guido Günther 2021-02-12  183  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35193 bytes --]

[-- Attachment #3: Type: text/plain, Size: 149 bytes --]

_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Guido Günther]

Hi ,
On Sat, Feb 13, 2021 at 11:12:37AM +0800, kernel test robot wrote:
> Hi "Guido,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on usb/usb-testing]
> [also build test WARNING on v5.11-rc7 next-20210211]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Guido-G-nther/usb-typec-tps6598x-Add-IRQ-flag-and-register-tracing/20210212-200855
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> config: openrisc-randconfig-s032-20210209 (attached as .config)
> compiler: or1k-linux-gcc (GCC) 9.3.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # apt-get install sparse
>         # sparse version: v0.6.3-215-g0fb77bb6-dirty
>         # https://github.com/0day-ci/linux/commit/ba45e1d5e1fd25b6aed8724106e6c7d5adef7a20
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Guido-G-nther/usb-typec-tps6598x-Add-IRQ-flag-and-register-tracing/20210212-200855
>         git checkout ba45e1d5e1fd25b6aed8724106e6c7d5adef7a20
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=openrisc 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> 
> 
> "sparse warnings: (new ones prefixed by >>)"
>    drivers/usb/typec/tps6598x.c: note: in included file (through include/trace/trace_events.h, include/trace/define_trace.h, drivers/usb/typec/tps6598x_trace.h):
> >> drivers/usb/typec/./tps6598x_trace.h:157:1: sparse: sparse: too long token expansion
> 

I looked around but didn't find any hints how to fix this. Any pointers
I missed (added the sparse list to cc:)?

Cheers,
 -- Guido

> vim +157 drivers/usb/typec/./tps6598x_trace.h
> 
> c90c0282e4ce33 Guido Günther 2021-02-12  156  
> ba45e1d5e1fd25 Guido Günther 2021-02-12 @157  TRACE_EVENT(tps6598x_status,
> ba45e1d5e1fd25 Guido Günther 2021-02-12  158  	    TP_PROTO(u32 status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  159  	    TP_ARGS(status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  160  
> ba45e1d5e1fd25 Guido Günther 2021-02-12  161  	    TP_STRUCT__entry(
> ba45e1d5e1fd25 Guido Günther 2021-02-12  162  			     __field(u32, status)
> ba45e1d5e1fd25 Guido Günther 2021-02-12  163  			     ),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  164  
> ba45e1d5e1fd25 Guido Günther 2021-02-12  165  	    TP_fast_assign(
> ba45e1d5e1fd25 Guido Günther 2021-02-12  166  			   __entry->status = status;
> ba45e1d5e1fd25 Guido Günther 2021-02-12  167  			   ),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  168  
> ba45e1d5e1fd25 Guido Günther 2021-02-12  169  	    TP_printk("conn: %s, pp_5v0: %s, pp_hv: %s, pp_ext: %s, pp_cable: %s, "
> ba45e1d5e1fd25 Guido Günther 2021-02-12  170  		      "pwr-src: %s, vbus: %s, usb-host: %s, legacy: %s, flags: %s",
> ba45e1d5e1fd25 Guido Günther 2021-02-12  171  		      show_status_conn_state(__entry->status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  172  		      show_status_pp_switch_state(TPS_STATUS_PP_5V0_SWITCH(__entry->status)),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  173  		      show_status_pp_switch_state(TPS_STATUS_PP_HV_SWITCH(__entry->status)),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  174  		      show_status_pp_switch_state(TPS_STATUS_PP_EXT_SWITCH(__entry->status)),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  175  		      show_status_pp_switch_state(TPS_STATUS_PP_CABLE_SWITCH(__entry->status)),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  176  		      show_status_power_sources(__entry->status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  177  		      show_status_vbus_status(__entry->status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  178  		      show_status_usb_host_present(__entry->status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  179  		      show_status_legacy(__entry->status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  180  		      show_status_flags(__entry->status)
> ba45e1d5e1fd25 Guido Günther 2021-02-12  181  		    )
> ba45e1d5e1fd25 Guido Günther 2021-02-12  182  );
> ba45e1d5e1fd25 Guido Günther 2021-02-12  183  
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org


> _______________________________________________
> kbuild mailing list -- kbuild@lists.01.org
> To unsubscribe send an email to kbuild-leave@lists.01.org

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Linus Torvalds]

On Sun, Feb 14, 2021 at 10:42 AM Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> >
> > I looked around but didn't find any hints how to fix this. Any pointers
> > I missed (added the sparse list to cc:)?
>
> This is a limitation of sparse; when using the 'stringize' pre-processor
> operator #, the maximum size of the resulting string is about 8k (if I
> remember correctly).

Well, yes and no.

The C89 standard actually says that a string literal can be at most
509 characters to be portable. C99 increased it to 4095 characters.

Sparse makes the limit higher, and the limit could easily be expanded
way past 8kB - but the point is that large string literals are
actually not guaranteed to be valid C.

So honestly, it really sounds like that TRACE_EVENT() thing is doing
something it shouldn't be doing.

I don't think there's any fundamental limit why sparse does 8kB as a
limit (just a few random buffers). Making sparse accept larger ones
should be as simple as just increasing MAX_STRING, but I really don't
think the kernel should encourage that kind of excessive string sizes.

I wouldn't be surprised if tracing buffers etc make such strings useless anyway.

               Linus

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Linus Torvalds]

On Sun, Feb 14, 2021 at 11:00 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The C89 standard actually says that a string literal can be at most
> 509 characters to be portable. C99 increased it to 4095 characters.
>
> Sparse makes the limit higher, and the limit could easily be expanded
> way past 8kB - but the point is that large string literals are
> actually not guaranteed to be valid C.

Looking around, there's a couple of other similar cases:

  drivers/infiniband/hw/hfi1/./trace_tx.h:727:1: error: too long token expansion
  arch/x86/purgatory/kexec-purgatory.c:1340:9: warning: trying to
concatenate 21400-character string (8191 bytes max)
  drivers/scsi/constants.c:318:9: warning: trying to concatenate
26550-character string (8191 bytes max)
  kernel/trace/trace.c:5290:1: warning: trying to concatenate
10842-character string (8191 bytes max)

but those four are the only ones I see from a quick x86-64 allmodconfig build.

Please try to avoid it.

          Linus

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Ramsay Jones]

On 14/02/2021 19:00, Linus Torvalds wrote:
> On Sun, Feb 14, 2021 at 10:42 AM Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>>
>>>
>>> I looked around but didn't find any hints how to fix this. Any pointers
>>> I missed (added the sparse list to cc:)?
>>
>> This is a limitation of sparse; when using the 'stringize' pre-processor
>> operator #, the maximum size of the resulting string is about 8k (if I
>> remember correctly).
> 
> Well, yes and no.
> 
> The C89 standard actually says that a string literal can be at most
> 509 characters to be portable. C99 increased it to 4095 characters.
> 
> Sparse makes the limit higher, and the limit could easily be expanded
> way past 8kB - but the point is that large string literals are
> actually not guaranteed to be valid C.
> 
> So honestly, it really sounds like that TRACE_EVENT() thing is doing
> something it shouldn't be doing.

Yep, as I said, I didn't submit the patch - rather I changed the source
so as not to need such a long string.

> I don't think there's any fundamental limit why sparse does 8kB as a
> limit (just a few random buffers). Making sparse accept larger ones
> should be as simple as just increasing MAX_STRING, but I really don't
> think the kernel should encourage that kind of excessive string sizes.

I agree, but I wiggled my patch (which doesn't increase MAX_STRING) to
apply to the current codebase, and ... it now fails two tests! ;-)
(It seems, in the intervening 9 years, the show_token_sequence() function
fixed the quoting of double-quotes in the resulting strings, which
my patch fails to do).

Sorry for the noise.

ATB,
Ramsay Jones

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Luc Van Oostenryck]

On Sun, Feb 14, 2021 at 11:00:48AM -0800, Linus Torvalds wrote:
> On Sun, Feb 14, 2021 at 10:42 AM Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
> >
> > >
> > > I looked around but didn't find any hints how to fix this. Any pointers
> > > I missed (added the sparse list to cc:)?
> >
> > This is a limitation of sparse; when using the 'stringize' pre-processor
> > operator #, the maximum size of the resulting string is about 8k (if I
> > remember correctly).
> 
> Well, yes and no.
> 
> The C89 standard actually says that a string literal can be at most
> 509 characters to be portable. C99 increased it to 4095 characters.
> 
> Sparse makes the limit higher, and the limit could easily be expanded
> way past 8kB - but the point is that large string literals are
> actually not guaranteed to be valid C.
> 
> So honestly, it really sounds like that TRACE_EVENT() thing is doing
> something it shouldn't be doing.

In itself, it's OKish but it does a lot of macro expansions and most
arguments are macros of macros of ... but the problem seems to be
limited to TP_printk().

In the current case, the offender is the string 'print_fmt_tps6598x_status'
which is just under 26K long especially because it expand
TPS6598X_STATUS_FLAGS_MASK but also because the arguments use FIELD_GET()
and thus __BF_FIELD_CHECK().
> 
> I don't think there's any fundamental limit why sparse does 8kB as a
> limit (just a few random buffers). Making sparse accept larger ones
> should be as simple as just increasing MAX_STRING, but I really don't
> think the kernel should encourage that kind of excessive string sizes.

Like you noted, there are just a few cases in the kernel and IIRC
there is or was one case in it too.
I would tend to increase MAX_STRING to something like 32 or 64K,
in order to keep it reasonable but let sparse to continue its processing,
but add a warning when the string/token is bigger than the current 8K.

-- Luc

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Guido Günther]

Hi,
On Sun, Feb 14, 2021 at 09:41:27PM +0100, Luc Van Oostenryck wrote:
> On Sun, Feb 14, 2021 at 11:00:48AM -0800, Linus Torvalds wrote:
> > On Sun, Feb 14, 2021 at 10:42 AM Ramsay Jones
> > <ramsay@ramsayjones.plus.com> wrote:
> > >
> > > >
> > > > I looked around but didn't find any hints how to fix this. Any pointers
> > > > I missed (added the sparse list to cc:)?
> > >
> > > This is a limitation of sparse; when using the 'stringize' pre-processor
> > > operator #, the maximum size of the resulting string is about 8k (if I
> > > remember correctly).
> > 
> > Well, yes and no.
> > 
> > The C89 standard actually says that a string literal can be at most
> > 509 characters to be portable. C99 increased it to 4095 characters.
> > 
> > Sparse makes the limit higher, and the limit could easily be expanded
> > way past 8kB - but the point is that large string literals are
> > actually not guaranteed to be valid C.
> > 
> > So honestly, it really sounds like that TRACE_EVENT() thing is doing
> > something it shouldn't be doing.
> 
> In itself, it's OKish but it does a lot of macro expansions and most
> arguments are macros of macros of ... but the problem seems to be
> limited to TP_printk().
> 
> In the current case, the offender is the string 'print_fmt_tps6598x_status'
> which is just under 26K long especially because it expand
> TPS6598X_STATUS_FLAGS_MASK but also because the arguments use FIELD_GET()
> and thus __BF_FIELD_CHECK().

That was a great hint! Using a custom FIELD_GET() that drops the
__BF_FIELD_CHECK() makes things fit.
Cheers,
 -- Guido

> > 
> > I don't think there's any fundamental limit why sparse does 8kB as a
> > limit (just a few random buffers). Making sparse accept larger ones
> > should be as simple as just increasing MAX_STRING, but I really don't
> > think the kernel should encourage that kind of excessive string sizes.
> 
> Like you noted, there are just a few cases in the kernel and IIRC
> there is or was one case in it too.
> I would tend to increase MAX_STRING to something like 32 or 64K,
> in order to keep it reasonable but let sparse to continue its processing,
> but add a warning when the string/token is bigger than the current 8K.
> 
> -- Luc
>