Re: [PATCH v3 4/4] usb: typec: tps6598x: Add trace event for data status
From: Heikki Krogerus
Date: Mon Mar 01 2021 - 10:16:30 EST
On Mon, Feb 15, 2021 at 12:46:45PM +0100, Guido Günther wrote:
> 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@xxxxxxxxxxx>
Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/typec/tps6598x.c | 12 ++++++-
> drivers/usb/typec/tps6598x.h | 38 +++++++++++++++++++++
> drivers/usb/typec/tps6598x_trace.h | 54 ++++++++++++++++++++++++++++++
> 3 files changed, 103 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 9a34c020f3e5..003a577be216 100644
> --- a/drivers/usb/typec/tps6598x.h
> +++ b/drivers/usb/typec/tps6598x.h
> @@ -148,4 +148,42 @@
> #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) \
> + TPS_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 \
> + TPS_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 \
> + TPS_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) | \
> + TPS_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
thanks,
--
heikki