[PATCH v2 6/7] usb: typec: displayport: Export probe and remove functions
From: Ajay Gupta
VirtualLink standard extends the DisplayPort Alt Mode by
utilizing also the USB 2 pins on the USB Type-C connector.
It uses the same messages as DisplayPort, but not the DP
SVID. At the time of writing, USB IF has not assigned a
Standard ID (SID) for VirtualLink, so the manufacturers of
VirtualLink adapters use their Vendor IDs as the SVID.
Since the SVID specific communication is exactly the same as
with DisplayPort alternate mode, there is no need to
implement separate driver for VirtualLink. We'll handle the
current VirtualLink adapters with probe drivers, and once
there is SVID assigned for it, we add it to the displayport
alt mode driver.
To support probing drivers, exporting the probe and remove
functions, and also changing the DP_HEADER helper macro to
use the SVID of the alternate mode device instead of the
DisplayPort alt mode SVID.
Suggested-by: Heikki Krogerus
Signed-off-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/altmodes/displayport.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/typec/altmodes/displayport.c
b/drivers/usb/typec/altmodes/displayport.c
index 1b2afeb1eeb6..4092248a5936 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -14,7 +14,7 @@
#include
#include
-#define DP_HEADER(cmd) (VDO(USB_TYPEC_DP_SID, 1, cmd) | \
+#define DP_HEADER(_dp, cmd)(VDO((_dp)->alt->svid, 1, cmd) | \
VDO_OPOS(USB_TYPEC_DP_MODE))
enum {
@@ -155,7 +155,7 @@ static int dp_altmode_configured(struct dp_altmode *dp)
static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf)
{
- u32 header = DP_HEADER(DP_CMD_CONFIGURE);
+ u32 header = DP_HEADER(dp, DP_CMD_CONFIGURE);
int ret;
ret = typec_altmode_notify(dp->alt, TYPEC_STATE_SAFE, &dp->data);
@@ -193,7 +193,7 @@ static void dp_altmode_work(struct work_struct *work)
dev_err(&dp->alt->dev, "failed to enter mode\n");
break;
case DP_STATE_UPDATE:
- header = DP_HEADER(DP_CMD_STATUS_UPDATE);
+ header = DP_HEADER(dp, DP_CMD_STATUS_UPDATE);
vdo = 1;
ret = typec_altmode_vdm(dp->alt, header, &vdo, 2);
if (ret)
@@ -507,7 +507,7 @@ static const struct attribute_group dp_altmode_group = {
.attrs = dp_altmode_attrs,
};
-static int dp_altmode_probe(struct typec_altmode *alt)
+int dp_altmode_probe(struct typec_altmode *alt)
{
const struct typec_altmode *port = typec_altmode_get_partner(alt);
struct dp_altmode *dp;
@@ -545,14 +545,16 @@ static int dp_altmode_probe(struct typec_altmode *alt)
return 0;
}
+EXPORT_SYMBOL_GPL(dp_altmode_probe);
-static void dp_altmode_remove(struct typec_altmode *alt)
+void dp_altmode_remove(struct typec_altmode *alt)
{
struct dp_altmode *dp = typec_altmode_get_drvdata(alt);
sysfs_remove_group(&alt->dev.kobj, &dp_altmode_group);
cancel_work_sync(&dp->work);
}
+EXPORT_SYMBOL_GPL(dp_altmode_remove);
static const struct typec_device_id dp_typec_id[] = {
{ USB_TYPEC_DP_SID, USB_TYPEC_DP_MODE },
--
2.20.1
[PATCH v2 1/7] i2c: nvidia-gpu: Supply CCGx driver the fw build info
From: Ajay Gupta
Adding device property "ccgx,firmware-build" for the CCGx
device, so the CCGx driver knows which firmware binary to
use for a specific vendor.
Suggested-by: Heikki Krogerus
Signed-off-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/i2c/busses/i2c-nvidia-gpu.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c
b/drivers/i2c/busses/i2c-nvidia-gpu.c
index 4e67d5ed480e..1c8f708f212b 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -253,6 +253,12 @@ static const struct pci_device_id gpu_i2c_ids[] = {
};
MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
+static const struct property_entry ccgx_props[] = {
+ /* Use FW built for NVIDIA (nv) only */
+ PROPERTY_ENTRY_U16("ccgx,firmware-build", ('n' << 8) | 'v'),
+ { }
+};
+
static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq)
{
struct i2c_client *ccgx_client;
@@ -267,6 +273,7 @@ static int gpu_populate_client(struct gpu_i2c_dev *i2cd,
int irq)
sizeof(i2cd->gpu_ccgx_ucsi->type));
i2cd->gpu_ccgx_ucsi->addr = 0x8;
i2cd->gpu_ccgx_ucsi->irq = irq;
+ i2cd->gpu_ccgx_ucsi->properties = ccgx_props;
ccgx_client = i2c_new_device(&i2cd->adapter, i2cd->gpu_ccgx_ucsi);
if (!ccgx_client)
return -ENODEV;
--
2.20.1
[PATCH v2 4/7] usb: typec: ucsi: Preliminary support for alternate modes
With UCSI the alternate modes, just like everything else
related to USB Type-C connectors, are handled in firmware.
The operating system can see the status and is allowed to
request certain things, for example entering and exiting the
modes, but the support for alternate modes is very limited
in UCSI. The feature is also optional, which means that even
when the platform supports alternate modes, the operating
system may not be even made aware of them.
UCSI does not support direct VDM reading or writing.
Instead, alternate modes can be entered and exited using a
single custom command which takes also an optional SVID
specific configuration value as parameter. That means every
supported alternate mode has to be handled separately in
UCSI driver.
This commit does not include support for any specific
alternate mode. The discovered alternate modes are now
registered, but binding a driver to an alternate mode will
not be possible until support for that alternate mode is
added to the UCSI driver.
Tested-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/trace.c | 12 ++
drivers/usb/typec/ucsi/trace.h | 26 +++
drivers/usb/typec/ucsi/ucsi.c | 358 +++--
drivers/usb/typec/ucsi/ucsi.h | 72 +++
4 files changed, 403 insertions(+), 65 deletions(-)
diff --git a/drivers/usb/typec/ucsi/trace.c b/drivers/usb/typec/ucsi/trace.c
index ffa3b4c3f338..1dabafb74320 100644
--- a/drivers/usb/typec/ucsi/trace.c
+++ b/drivers/usb/typec/ucsi/trace.c
@@ -60,3 +60,15 @@ const char *ucsi_cci_str(u32 cci)
return "";
}
+
+static const char * const ucsi_recipient_strs[] = {
+ [UCSI_RECIPIENT_CON]= "port",
+ [UCSI_RECIPIENT_SOP]= "partner",
+ [UCSI_RECIPIENT_SOP_P] = "plug (prime)",
+ [UCSI_RECIPIENT_SOP_PP] = "plug (double prime)",
+};
+
+const char *ucsi_recipient_str(u8 recipient)
+{
+ return ucsi_recipient_strs[recipient];
+}
diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h
index 5e2906df2db7..783ec9c72055 100644
--- a/drivers/usb/typec/ucsi/trace.h
+++ b/drivers/usb/typec/ucsi/trace.h
@@ -7,6 +7,7 @@
#define __UCSI_TRACE_H
#include
+#include
const char *ucsi_cmd_str(u64 raw_cmd);
const char *ucsi_ack_str(u8 ack);
@@ -134,6 +135,31 @@ DEFINE_EVENT(ucsi_log_connector_status, ucsi_register_port,
TP_ARGS(port, status)
);
+DECLARE_EVENT_CLASS(ucsi_log_register_altmode,
+ TP_PROTO(u8 recipient, struct typec_altmode *alt),
+ TP_ARGS(recipient, alt),
+ TP_STRUCT__entry(
+ __field(u8, recipient)
+ __field(u16, svid)
+ __field(u8, mode)
+ __field(u32, vdo)
+ ),
+ TP_fast_assign(
+ __entry->recipient = recipient;
+ __entry->svid = alt->svid;
+ __entry->mode = alt->mode;
+ __entry->vdo = alt->vdo;
+ ),
+ TP_printk("%s alt mode: svid %04x, mode %d vdo %x",
+ ucsi_recipient_str(__entry->recipient), __entry->svid,
+ __entry->mode, __entry->vdo)
+);
+
+DEFINE_EVENT(ucsi_log_register_altmode, ucsi_register_altmode,
+ TP_PROTO(u8 recipient, struct typec_altmode *alt),
+ TP_ARGS(recipient, alt)
+);
+
#endif /* __UCSI_TRACE_H */
/* This part must be outside protection */
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 8d0a6fe748bd..585ab04be940 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -12,7 +12,7 @@
#include
#include
#include
-#include
+#include
#include "ucsi.h"
#include "trace.h"
@@ -45,22 +45,6 @@ enum ucsi_status {
UCSI_ERROR,
};
-struct ucsi_connector {
- int num;
-
- struct ucsi *ucsi;
- struct work_struct work;
- struct completion complete;
-
- struct typec_port *port;
- struct typec_partner *partner;
-
- struct typec_capability typec_cap;
-
- struct ucsi_connector_status status;
- struct ucsi_connector_capability cap;
-};
-
struct ucsi {
struct device *dev;
struct ucsi_ppm *ppm;
@@ -238,8 +222,204 @@ static int ucsi_run_command(struct ucsi *ucsi, struct
ucsi_control *ctrl,
return ret;
}
+int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
+ void *retval, size_t size)
+{
+ int ret;
+
+ mutex_lock(&ucsi->ppm_lock);
+ ret = ucsi_run_command(ucsi, ctrl, retval, size);
+ mutex_unlock(&ucsi->ppm_lock);
+
+ return ret;
+}
+
/* --
*/
+void ucsi_altmode_update_active(struct ucsi_connector *con)
+{
+ const struct typec_altmode *altmode = NULL;
+ struct ucsi_control ctrl;
+ int ret;
+ u8 cur;
+
[PATCH v2 5/7] usb: typec: ucsi: Support for DisplayPort alt mode
This makes it possible to bind a driver to a DisplayPort
alt mode adapter devices.
The driver attempts to cope with the limitations of UCSI by
"emulating" behaviour and attempting to guess things when
ever possible in order to satisfy the requirements the
standard DisplayPort alt mode driver has.
Tested-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/Makefile | 15 +-
drivers/usb/typec/ucsi/displayport.c | 305 +++
drivers/usb/typec/ucsi/ucsi.c| 21 +-
drivers/usb/typec/ucsi/ucsi.h| 21 ++
4 files changed, 354 insertions(+), 8 deletions(-)
create mode 100644 drivers/usb/typec/ucsi/displayport.c
diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
index 2f4900b26210..b35e15a1f02c 100644
--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile
@@ -1,12 +1,15 @@
# SPDX-License-Identifier: GPL-2.0
-CFLAGS_trace.o := -I$(src)
+CFLAGS_trace.o := -I$(src)
-obj-$(CONFIG_TYPEC_UCSI) += typec_ucsi.o
+obj-$(CONFIG_TYPEC_UCSI) += typec_ucsi.o
-typec_ucsi-y := ucsi.o
+typec_ucsi-y := ucsi.o
-typec_ucsi-$(CONFIG_TRACING) += trace.o
+typec_ucsi-$(CONFIG_TRACING) += trace.o
-obj-$(CONFIG_UCSI_ACPI)+= ucsi_acpi.o
+ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
+ typec_ucsi-y+= displayport.o
+endif
-obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
+obj-$(CONFIG_UCSI_ACPI)+= ucsi_acpi.o
+obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
diff --git a/drivers/usb/typec/ucsi/displayport.c
b/drivers/usb/typec/ucsi/displayport.c
new file mode 100644
index ..6c892cb409b7
--- /dev/null
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UCSI DisplayPort Alternate Mode Support
+ *
+ * Copyright (C) 2018, Intel Corporation
+ * Author: Heikki Krogerus
+ */
+
+#include
+#include
+
+#include "ucsi.h"
+
+#define UCSI_CMD_SET_NEW_CAM(_con_num_, _enter_, _cam_, _am_) \
+(UCSI_SET_NEW_CAM | ((_con_num_) << 16) | ((_enter_) << 23) | \
+ ((_cam_) << 24) | ((u64)(_am_) << 32))
+
+struct ucsi_dp {
+ struct typec_displayport_data data;
+ struct ucsi_connector *con;
+ struct typec_altmode *alt;
+ struct work_struct work;
+ int offset;
+
+ bool override;
+ bool initialized;
+
+ u32 header;
+ u32 *vdo_data;
+ u8 vdo_size;
+};
+
+/*
+ * Note. Alternate mode control is optional feature in UCSI. It means that even
+ * if the system supports alternate modes, the OS may not be aware of them.
+ *
+ * In most cases however, the OS will be able to see the supported alternate
+ * modes, but it may still not be able to configure them, not even enter or
exit
+ * them. That is because UCSI defines alt mode details and alt mode
"overriding"
+ * as separate options.
+ *
+ * In case alt mode details are supported, but overriding is not, the driver
+ * will still display the supported pin assignments and configuration, but any
+ * changes the user attempts to do will lead into failure with return value of
+ * -EOPNOTSUPP.
+ */
+
+static int ucsi_displayport_enter(struct typec_altmode *alt)
+{
+ struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
+
+ mutex_lock(&dp->con->lock);
+
+ if (!dp->override && dp->initialized) {
+ const struct typec_altmode *p = typec_altmode_get_partner(alt);
+
+ dev_warn(&p->dev,
+"firmware doesn't support alternate mode
overriding\n");
+ mutex_unlock(&dp->con->lock);
+ return -EOPNOTSUPP;
+ }
+
+ /*
+* We can't send the New CAM command yet to the PPM as it needs the
+* configuration value as well. Pretending that we have now entered the
+* mode, and letting the alt mode driver continue.
+*/
+
+ dp->header = VDO(USB_TYPEC_DP_SID, 1, CMD_ENTER_MODE);
+ dp->header |= VDO_OPOS(USB_TYPEC_DP_MODE);
+ dp->header |= VDO_CMDT(CMDT_RSP_ACK);
+
+ dp->vdo_data = NULL;
+ dp->vdo_size = 1;
+
+ schedule_work(&dp->work);
+
+ mutex_unlock(&dp->con->lock);
+
+ return 0;
+}
+
+static int ucsi_displayport_exit(struct typec_altmode *alt)
+{
+ struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
+ struct ucsi_control ctrl;
+ int ret = 0;
+ u8 cur;
+
+ mutex_lock(&dp->con->lock);
+
+ if (!dp->override) {
+ const struct typec_altmode *p = typec_altmode_get_partner(alt);
+
+ dev_warn(&p->dev,
+"firmware doesn't support alternate mode
overriding\n"
[PATCH v2 3/7] usb: typec: ucsi: ccg: add firmware flashing support
From: Ajay Gupta
CCGx has two copies of the firmware in addition to the bootloader.
If the device is running FW1, FW2 can be updated with the new version.
Dual firmware mode allows the CCG device to stay in a PD contract and
support USB PD and Type-C functionality while a firmware update is in
progress.
First we read the currently flashed firmware version of both
primary and secondary firmware and then compare it with
version of firmware file to determine if flashing is required.
Command framework is added to support sending commands to CCGx
controller. We wait for response after sending the command and then
read the response from RAB_RESPONSE register.
Below commands are supported,
- ENTER_FLASHING
- RESET
- PDPORT_ENABLE
- JUMP_TO_BOOT
- FLASH_ROW_RW
- VALIDATE_FW
Command specific mutex lock is also added to sync between driver
and user threads.
PD port number information is added which is required while sending
PD_PORT_ENABLE command
Signed-off-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/ucsi_ccg.c | 839 +-
1 file changed, 826 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 3884fb41c72e..44cadb5d18d3 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -9,6 +9,7 @@
*/
#include
#include
+#include
#include
#include
#include
@@ -24,6 +25,73 @@ enum enum_fw_mode {
FW_INVALID,
};
+#define CCGX_RAB_DEVICE_MODE 0x
+#define CCGX_RAB_INTR_REG 0x0006
+#define DEV_INT BIT(0)
+#define PORT0_INT BIT(1)
+#define PORT1_INT BIT(2)
+#define UCSI_READ_INT BIT(7)
+#define CCGX_RAB_JUMP_TO_BOOT 0x0007
+#define TO_BOOT 'J'
+#define TO_ALT_FW 'A'
+#define CCGX_RAB_RESET_REQ 0x0008
+#define RESET_SIG 'R'
+#define CMD_RESET_I2C 0x0
+#define CMD_RESET_DEV 0x1
+#define CCGX_RAB_ENTER_FLASHING0x000A
+#define FLASH_ENTER_SIG 'P'
+#define CCGX_RAB_VALIDATE_FW 0x000B
+#define CCGX_RAB_FLASH_ROW_RW 0x000C
+#define FLASH_SIG 'F'
+#define FLASH_RD_CMD 0x0
+#define FLASH_WR_CMD 0x1
+#define FLASH_FWCT1_WR_CMD0x2
+#define FLASH_FWCT2_WR_CMD0x3
+#define FLASH_FWCT_SIG_WR_CMD 0x4
+#define CCGX_RAB_READ_ALL_VER 0x0010
+#define CCGX_RAB_READ_FW2_VER 0x0020
+#define CCGX_RAB_UCSI_CONTROL 0x0039
+#define CCGX_RAB_UCSI_CONTROL_STARTBIT(0)
+#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
+#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
+#define REG_FLASH_RW_MEM0x0200
+#define DEV_REG_IDXCCGX_RAB_DEVICE_MODE
+#define CCGX_RAB_PDPORT_ENABLE 0x002C
+#define PDPORT_1 BIT(0)
+#define PDPORT_2 BIT(1)
+#define CCGX_RAB_RESPONSE 0x007E
+#define ASYNC_EVENT BIT(7)
+
+/* CCGx events & async msg codes */
+#define RESET_COMPLETE 0x80
+#define EVENT_INDEXRESET_COMPLETE
+#define PORT_CONNECT_DET 0x84
+#define PORT_DISCONNECT_DET0x85
+#define ROLE_SWAP_COMPELETE0x87
+
+/* ccg firmware */
+#define CYACD_LINE_SIZE 527
+#define CCG4_ROW_SIZE 256
+#define FW1_METADATA_ROW0x1FF
+#define FW2_METADATA_ROW0x1FE
+#define FW_CFG_TABLE_SIG_SIZE 256
+
+static int secondary_fw_min_ver = 41;
+
+enum enum_flash_mode {
+ SECONDARY_BL, /* update secondary using bootloader */
+ PRIMARY,/* update primary using secondary */
+ SECONDARY, /* update secondary using primary */
+ FLASH_NOT_NEEDED, /* update not required */
+ FLASH_INVALID,
+};
+
+static const char * const ccg_fw_names[] = {
+ "ccg_boot.cyacd",
+ "ccg_primary.cyacd",
+ "ccg_secondary.cyacd"
+};
+
struct ccg_dev_info {
#define CCG_DEVINFO_FWMODE_SHIFT (0)
#define CCG_DEVINFO_FWMODE_MASK (0x3 << CCG_DEVINFO_FWMODE_SHIFT)
@@ -50,6 +118,50 @@ struct version_info {
struct version_format app;
};
+struct fw_config_table {
+ u32 identity;
+ u16 table_size;
+ u8 fwct_version;
+ u8 is_key_change;
+ u8 guid[16];
+ struct version_format base;
+ struct version_format app;
+ u8 primary_fw_digest[32];
+ u32 key_exp_leng
[PATCH v2 7/7] usb: typec: Add driver for NVIDIA Alt Modes
From: Ajay Gupta
Latest NVIDIA GPUs support VirtualLink device. Since USBIF
has not assigned a Standard ID (SID) for VirtualLink
so using NVIDA VID 0x955 as SVID.
Signed-off-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/altmodes/Kconfig | 10 +++
drivers/usb/typec/altmodes/Makefile | 2 ++
drivers/usb/typec/altmodes/nvidia.c | 44 +
drivers/usb/typec/ucsi/ucsi.c | 4 ++-
include/linux/usb/typec_dp.h| 5
5 files changed, 64 insertions(+), 1 deletion(-)
create mode 100644 drivers/usb/typec/altmodes/nvidia.c
diff --git a/drivers/usb/typec/altmodes/Kconfig
b/drivers/usb/typec/altmodes/Kconfig
index ef2226eb7a33..187690fd1a5b 100644
--- a/drivers/usb/typec/altmodes/Kconfig
+++ b/drivers/usb/typec/altmodes/Kconfig
@@ -12,4 +12,14 @@ config TYPEC_DP_ALTMODE
To compile this driver as a module, choose M here: the
module will be called typec_displayport.
+config TYPEC_NVIDIA_ALTMODE
+ tristate "NVIDIA Alternate Mode driver"
+ depends on TYPEC_DP_ALTMODE
+ help
+ Latest NVIDIA GPUs support VirtualLink devices. Select this
+ to enable support for VirtualLink devices with NVIDIA GPUs.
+
+ To compile this driver as a module, choose M here: the
+ module will be called typec_displayport.
+
endmenu
diff --git a/drivers/usb/typec/altmodes/Makefile
b/drivers/usb/typec/altmodes/Makefile
index eda8456f1c92..45717548b396 100644
--- a/drivers/usb/typec/altmodes/Makefile
+++ b/drivers/usb/typec/altmodes/Makefile
@@ -2,3 +2,5 @@
obj-$(CONFIG_TYPEC_DP_ALTMODE) += typec_displayport.o
typec_displayport-y:= displayport.o
+obj-$(CONFIG_TYPEC_NVIDIA_ALTMODE) += typec_nvidia.o
+typec_nvidia-y := nvidia.o
diff --git a/drivers/usb/typec/altmodes/nvidia.c
b/drivers/usb/typec/altmodes/nvidia.c
new file mode 100644
index ..c36769736405
--- /dev/null
+++ b/drivers/usb/typec/altmodes/nvidia.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 NVIDIA Corporation. All rights reserved.
+ *
+ * NVIDIA USB Type-C Alt Mode Driver
+ */
+#include
+#include
+#include
+#include "displayport.h"
+
+static int nvidia_altmode_probe(struct typec_altmode *alt)
+{
+ if (alt->svid == USB_TYPEC_NVIDIA_VLINK_SID)
+ return dp_altmode_probe(alt);
+ else
+ return -ENOTSUPP;
+}
+
+static void nvidia_altmode_remove(struct typec_altmode *alt)
+{
+ if (alt->svid == USB_TYPEC_NVIDIA_VLINK_SID)
+ dp_altmode_remove(alt);
+}
+
+static const struct typec_device_id nvidia_typec_id[] = {
+ { USB_TYPEC_NVIDIA_VLINK_SID, TYPEC_ANY_MODE },
+ { },
+};
+MODULE_DEVICE_TABLE(typec, nvidia_typec_id);
+
+static struct typec_altmode_driver nvidia_altmode_driver = {
+ .id_table = nvidia_typec_id,
+ .probe = nvidia_altmode_probe,
+ .remove = nvidia_altmode_remove,
+ .driver = {
+ .name = "typec_nvidia",
+ .owner = THIS_MODULE,
+ },
+};
+module_typec_altmode_driver(nvidia_altmode_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("NVIDIA USB Type-C Alt Mode Driver");
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 22fb956687de..999ae50d6e49 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -310,6 +310,7 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
switch (desc->svid) {
case USB_TYPEC_DP_SID:
+ case USB_TYPEC_NVIDIA_VLINK_SID:
alt = ucsi_register_displayport(con, override, i, desc);
break;
default:
@@ -428,7 +429,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector
*con, u8 recipient)
while (adev[i]) {
if (recipient == UCSI_RECIPIENT_SOP &&
- adev[i]->svid == USB_TYPEC_DP_SID) {
+ (adev[i]->svid == USB_TYPEC_DP_SID ||
+ adev[i]->svid == USB_TYPEC_NVIDIA_VLINK_SID)) {
pdev = typec_altmode_get_partner(adev[i]);
ucsi_displayport_remove_partner((void *)pdev);
}
diff --git a/include/linux/usb/typec_dp.h b/include/linux/usb/typec_dp.h
index 7fa12ef8d09a..fc4c7edb2e8a 100644
--- a/include/linux/usb/typec_dp.h
+++ b/include/linux/usb/typec_dp.h
@@ -5,6 +5,11 @@
#include
#define USB_TYPEC_DP_SID 0xff01
+/* USB IF has not assigned a Standard ID (SID) for VirtualLink,
+ * so the manufacturers of VirtualLink adapters use their Vendor
+ * IDs as the SVID.
+ */
+#define USB_TYPEC_NVIDIA_VLINK_SID 0x955 /* NVIDIA VirtualLink */
#define USB_TYPEC_DP_MODE 1
/*
--
2.20.1
[PATCH v2 2/7] usb: typec: ucsi: ccg: add get_fw_info function
From: Ajay Gupta
Function is to get the details of ccg firmware and device version.
It will be useful in debugging and also during firmware update.
Signed-off-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/ucsi_ccg.c | 66 ++-
1 file changed, 64 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
b/drivers/usb/typec/ucsi/ucsi_ccg.c
index de8a43bdff68..3884fb41c72e 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -17,15 +17,54 @@
#include
#include "ucsi.h"
+enum enum_fw_mode {
+ BOOT, /* bootloader */
+ FW1,/* FW partition-1 (contains secondary fw) */
+ FW2,/* FW partition-2 (contains primary fw) */
+ FW_INVALID,
+};
+
+struct ccg_dev_info {
+#define CCG_DEVINFO_FWMODE_SHIFT (0)
+#define CCG_DEVINFO_FWMODE_MASK (0x3 << CCG_DEVINFO_FWMODE_SHIFT)
+#define CCG_DEVINFO_PDPORTS_SHIFT (2)
+#define CCG_DEVINFO_PDPORTS_MASK (0x3 << CCG_DEVINFO_PDPORTS_SHIFT)
+ u8 mode;
+ u8 bl_mode;
+ __le16 silicon_id;
+ __le16 bl_last_row;
+} __packed;
+
+struct version_format {
+ __le16 build;
+ u8 patch;
+ u8 ver;
+#define CCG_VERSION_MIN_SHIFT (0)
+#define CCG_VERSION_MIN_MASK (0xf << CCG_VERSION_MIN_SHIFT)
+#define CCG_VERSION_MAJ_SHIFT (4)
+#define CCG_VERSION_MAJ_MASK (0xf << CCG_VERSION_MAJ_SHIFT)
+} __packed;
+
+struct version_info {
+ struct version_format base;
+ struct version_format app;
+};
+
struct ucsi_ccg {
struct device *dev;
struct ucsi *ucsi;
struct ucsi_ppm ppm;
struct i2c_client *client;
+ struct ccg_dev_info info;
+ /* version info for boot, primary and secondary */
+ struct version_info version[FW2 + 1];
};
-#define CCGX_RAB_INTR_REG 0x06
-#define CCGX_RAB_UCSI_CONTROL 0x39
+#define CCGX_RAB_DEVICE_MODE 0x
+#define CCGX_RAB_INTR_REG 0x0006
+#define CCGX_RAB_READ_ALL_VER 0x0010
+#define CCGX_RAB_READ_FW2_VER 0x0020
+#define CCGX_RAB_UCSI_CONTROL 0x0039
#define CCGX_RAB_UCSI_CONTROL_STARTBIT(0)
#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
@@ -220,6 +259,23 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
return IRQ_HANDLED;
}
+static int get_fw_info(struct ucsi_ccg *uc)
+{
+ int err;
+
+ err = ccg_read(uc, CCGX_RAB_READ_ALL_VER, (u8 *)(&uc->version),
+ sizeof(uc->version));
+ if (err < 0)
+ return err;
+
+ err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
+ sizeof(uc->info));
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
static int ucsi_ccg_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -248,6 +304,12 @@ static int ucsi_ccg_probe(struct i2c_client *client,
return status;
}
+ status = get_fw_info(uc);
+ if (status < 0) {
+ dev_err(uc->dev, "get_fw_info failed - %d\n", status);
+ return status;
+ }
+
status = devm_request_threaded_irq(dev, client->irq, NULL,
ccg_irq_handler,
IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
--
2.20.1
[RFC PATCH 1/2] i2c: nvidia-gpu: Supply CCGx driver the firmware build information
Adding device property "ccgx,firmware-build" for the CCGx
device, so the CCGx driver knows the which firmware build it
needs to be dealing with.
Signed-off-by: Heikki Krogerus
---
drivers/i2c/busses/i2c-nvidia-gpu.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c
b/drivers/i2c/busses/i2c-nvidia-gpu.c
index 4e67d5ed480e..5604323ee0b9 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -253,6 +253,11 @@ static const struct pci_device_id gpu_i2c_ids[] = {
};
MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
+static const struct property_entry ccgx_props[] = {
+ PROPERTY_ENTRY_U16("ccgx,firmware-build", 0x766e),
+ { }
+};
+
static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq)
{
struct i2c_client *ccgx_client;
@@ -267,6 +272,7 @@ static int gpu_populate_client(struct gpu_i2c_dev *i2cd,
int irq)
sizeof(i2cd->gpu_ccgx_ucsi->type));
i2cd->gpu_ccgx_ucsi->addr = 0x8;
i2cd->gpu_ccgx_ucsi->irq = irq;
+ i2cd->gpu_ccgx_ucsi->properties = ccgx_props;
ccgx_client = i2c_new_device(&i2cd->adapter, i2cd->gpu_ccgx_ucsi);
if (!ccgx_client)
return -ENODEV;
--
2.20.1
[RFC PATCH 0/2] usb: typec: ucsi: ccgx: FW build device property
Hi Ajay,
This the example I promised. I prepared these on top of your
patches. The second patch ("usb: typec: ucsi: ccgx: Read the
fw build property") I want to squash into your patch adding
the firmware handing to the CCGx driver.
If you want to add modifications to these, resend the
series. Otherwise if you are OK with these, I'll squash the
first one into your patch, and resend everything to Greg.
I'll also picked the VirtualLink patches.
cheers,
Heikki Krogerus (2):
i2c: nvidia-gpu: Supply CCGx driver the firmware build information
usb: typec: ucsi: ccgx: Read the fw build property
drivers/i2c/busses/i2c-nvidia-gpu.c | 6 ++
drivers/usb/typec/ucsi/ucsi_ccg.c | 23 ---
2 files changed, 22 insertions(+), 7 deletions(-)
--
2.20.1
[RFC PATCH 2/2] usb: typec: ucsi: ccgx: Read the fw build property
Ajay, please squash this into your patch if it's OK.
I took the liberty of removing one label that was not used.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/ucsi_ccg.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 5508beec08e3..50e5ac1d2d0a 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -180,6 +180,8 @@ struct ucsi_ccg {
int irq;
struct work_struct work;
struct mutex lock; /* to sync between user and driver thread */
+
+ u16 fw_build;
};
static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
@@ -687,7 +689,7 @@ static bool ccg_check_vendor_version(struct ucsi_ccg *uc,
/* Check if the fw build is for supported vendors.
* Add all supported vendors here.
*/
- if (app->build != (('n' << 8) | 'v')) {
+ if (app->build != uc->fw_build) {
dev_info(dev, "current fw is not from supported vendor\n");
return false;
}
@@ -695,7 +697,7 @@ static bool ccg_check_vendor_version(struct ucsi_ccg *uc,
/* Check if the new fw build is for supported vendors
* Add all supported vendors here.
*/
- if (fw_cfg->app.build != (('n' << 8) | 'v')) {
+ if (fw_cfg->app.build != uc->fw_build) {
dev_info(dev, "new fw is not from supported vendor\n");
return false;
}
@@ -721,14 +723,14 @@ static bool ccg_check_fw_version(struct ucsi_ccg *uc,
const char *fw_name,
* last part of fw image is fw cfg table and signature
*/
if (fw->size < sizeof(fw_cfg) + FW_CFG_TABLE_SIG_SIZE)
- goto not_signed_fw;
+ goto out_release_firmware;
memcpy((uint8_t *)&fw_cfg, fw->data + fw->size -
sizeof(fw_cfg) - FW_CFG_TABLE_SIG_SIZE, sizeof(fw_cfg));
if (fw_cfg.identity != ('F' | 'W' << 8 | 'C' << 16 | 'T' << 24)) {
dev_info(dev, "not a signed image\n");
- goto not_signed_fw;
+ goto out_release_firmware;
}
/* compare input version with FWCT version */
@@ -738,13 +740,12 @@ static bool ccg_check_fw_version(struct ucsi_ccg *uc,
const char *fw_name,
fw_cfg.app.ver << 24;
if (!ccg_check_vendor_version(uc, app, &fw_cfg))
- goto not_supported_version;
+ goto out_release_firmware;
if (new_version > cur_version)
is_later = true;
-not_supported_version:
-not_signed_fw:
+out_release_firmware:
release_firmware(fw);
return is_later;
}
@@ -1085,6 +1086,14 @@ static int ucsi_ccg_probe(struct i2c_client *client,
mutex_init(&uc->lock);
INIT_WORK(&uc->work, ccg_update_firmware);
+ /* REVISIT: This probable does not need to be fatal. */
+ status = device_property_read_u16(dev, "ccgx,firmware-build",
+ &uc->fw_build);
+ if (status) {
+ dev_err(uc->dev, "faild to get FW build\n");
+ return status;
+ }
+
/* reset ccg device and initialize ucsi */
status = ucsi_ccg_init(uc);
if (status < 0) {
--
2.20.1
Re: [PATCH v6 2/2] usb: typec: ucsi: ccg: add firmware flashing support
Hi Ajay,
On Thu, Apr 11, 2019 at 01:03:10PM -0700, Ajay Gupta wrote:
> +#define CCG_FW_VERSION_NVIDIA(b) (le16_to_cpu(b) != (('n' << 8) | 'v'))
Does that remove the sparce warning?
Now that I see that this version is specific to NVIDIA, I think we are
going to want to get this detail from a device property. I'll prepare
an example for you right now so you can see how it can be done.
> +static bool ccg_check_vendor_version(struct ucsi_ccg *uc,
> + struct version_format *app,
> + struct fw_config_table *fw_cfg)
> +{
> + struct device *dev = uc->dev;
> +
> + /* Check if the fw build is for supported vendors.
> + * Add all supported vendors here.
> + */
> + if (CCG_FW_VERSION_NVIDIA(app->build)) {
> + dev_info(dev, "current fw is not from supported vendor\n");
> + return false;
> + }
> +
> + /* Check if the new fw build is for supported vendors
> + * Add all supported vendors here.
> + */
> + if (CCG_FW_VERSION_NVIDIA(fw_cfg->app.build)) {
> + dev_info(dev, "new fw is not from supported vendor\n");
> + return false;
> + }
> + return true;
> +}
Br,
--
heikki
Re: [PATCH 2/4] usb: typec: ucsi: ccg: add firmware flashing support
Hi Ajay,
On Thu, Apr 11, 2019 at 12:31:45AM +0800, kbuild test robot wrote:
> Hi Heikki,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on usb/usb-testing]
> [also build test WARNING on v5.1-rc4 next-20190410]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Heikki-Krogerus/usb-typec-ucsi-Remaining-changes-for-v5-2/20190410-221455
> base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> usb-testing
> reproduce:
> # apt-get install sparse
> make ARCH=x86_64 allmodconfig
> make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
>
>
> sparse warnings: (new ones prefixed by >>)
>
>drivers/usb/typec/ucsi/ucsi_ccg.c:212:24: sparse: expression using
> sizeof(void)
>drivers/usb/typec/ucsi/ucsi_ccg.c:212:24: sparse: expression using
> sizeof(void)
> >> drivers/usb/typec/ucsi/ucsi_ccg.c:690:16: sparse: restricted __le16
> >> degrades to integer
>drivers/usb/typec/ucsi/ucsi_ccg.c:698:24: sparse: restricted __le16
> degrades to integer
>drivers/usb/typec/ucsi/ucsi_ccg.c:735:26: sparse: restricted __le16
> degrades to integer
>drivers/usb/typec/ucsi/ucsi_ccg.c:737:33: sparse: restricted __le16
> degrades to integer
>drivers/usb/typec/ucsi/ucsi_ccg.c:777:37: sparse: restricted __le16
> degrades to integer
>
> vim +690 drivers/usb/typec/ucsi/ucsi_ccg.c
>
>680
>681static bool ccg_check_vendor_version(struct ucsi_ccg *uc,
>682 struct version_format *app,
>683 struct fw_config_table
> *fw_cfg)
>684{
>685struct device *dev = uc->dev;
>686
>687/* Check if the fw build is for supported vendors.
>688 * Add all supported vendors here.
>689 */
> > 690if (app->build != (('n' << 8) | 'v')) {
How about a macro for these?
#define CCG_VERSION_BUILD (__le16)(...)
>691dev_info(dev, "current fw is not from supported
> vendor\n");
>692return false;
>693}
>694
>695/* Check if the new fw build is for supported vendors
>696 * Add all supported vendors here.
>697 */
>698if (fw_cfg->app.build != (('n' << 8) | 'v')) {
>699dev_info(dev, "new fw is not from supported
> vendor\n");
>700return false;
>701}
>702return true;
>703}
>704
thanks,
--
heikki
[PATCH 3/4] usb: typec: ucsi: Preliminary support for alternate modes
With UCSI the alternate modes, just like everything else
related to USB Type-C connectors, are handled in firmware.
The operating system can see the status and is allowed to
request certain things, for example entering and exiting the
modes, but the support for alternate modes is very limited
in UCSI. The feature is also optional, which means that even
when the platform supports alternate modes, the operating
system may not be even made aware of them.
UCSI does not support direct VDM reading or writing.
Instead, alternate modes can be entered and exited using a
single custom command which takes also an optional SVID
specific configuration value as parameter. That means every
supported alternate mode has to be handled separately in
UCSI driver.
This commit does not include support for any specific
alternate mode. The discovered alternate modes are now
registered, but binding a driver to an alternate mode will
not be possible until support for that alternate mode is
added to the UCSI driver.
Tested-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/trace.c | 12 ++
drivers/usb/typec/ucsi/trace.h | 26 +++
drivers/usb/typec/ucsi/ucsi.c | 354 +++--
drivers/usb/typec/ucsi/ucsi.h | 72 +++
4 files changed, 399 insertions(+), 65 deletions(-)
diff --git a/drivers/usb/typec/ucsi/trace.c b/drivers/usb/typec/ucsi/trace.c
index ffa3b4c3f338..1dabafb74320 100644
--- a/drivers/usb/typec/ucsi/trace.c
+++ b/drivers/usb/typec/ucsi/trace.c
@@ -60,3 +60,15 @@ const char *ucsi_cci_str(u32 cci)
return "";
}
+
+static const char * const ucsi_recipient_strs[] = {
+ [UCSI_RECIPIENT_CON]= "port",
+ [UCSI_RECIPIENT_SOP]= "partner",
+ [UCSI_RECIPIENT_SOP_P] = "plug (prime)",
+ [UCSI_RECIPIENT_SOP_PP] = "plug (double prime)",
+};
+
+const char *ucsi_recipient_str(u8 recipient)
+{
+ return ucsi_recipient_strs[recipient];
+}
diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h
index 5e2906df2db7..783ec9c72055 100644
--- a/drivers/usb/typec/ucsi/trace.h
+++ b/drivers/usb/typec/ucsi/trace.h
@@ -7,6 +7,7 @@
#define __UCSI_TRACE_H
#include
+#include
const char *ucsi_cmd_str(u64 raw_cmd);
const char *ucsi_ack_str(u8 ack);
@@ -134,6 +135,31 @@ DEFINE_EVENT(ucsi_log_connector_status, ucsi_register_port,
TP_ARGS(port, status)
);
+DECLARE_EVENT_CLASS(ucsi_log_register_altmode,
+ TP_PROTO(u8 recipient, struct typec_altmode *alt),
+ TP_ARGS(recipient, alt),
+ TP_STRUCT__entry(
+ __field(u8, recipient)
+ __field(u16, svid)
+ __field(u8, mode)
+ __field(u32, vdo)
+ ),
+ TP_fast_assign(
+ __entry->recipient = recipient;
+ __entry->svid = alt->svid;
+ __entry->mode = alt->mode;
+ __entry->vdo = alt->vdo;
+ ),
+ TP_printk("%s alt mode: svid %04x, mode %d vdo %x",
+ ucsi_recipient_str(__entry->recipient), __entry->svid,
+ __entry->mode, __entry->vdo)
+);
+
+DEFINE_EVENT(ucsi_log_register_altmode, ucsi_register_altmode,
+ TP_PROTO(u8 recipient, struct typec_altmode *alt),
+ TP_ARGS(recipient, alt)
+);
+
#endif /* __UCSI_TRACE_H */
/* This part must be outside protection */
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 8d0a6fe748bd..11d1884322c2 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -12,7 +12,7 @@
#include
#include
#include
-#include
+#include
#include "ucsi.h"
#include "trace.h"
@@ -45,22 +45,6 @@ enum ucsi_status {
UCSI_ERROR,
};
-struct ucsi_connector {
- int num;
-
- struct ucsi *ucsi;
- struct work_struct work;
- struct completion complete;
-
- struct typec_port *port;
- struct typec_partner *partner;
-
- struct typec_capability typec_cap;
-
- struct ucsi_connector_status status;
- struct ucsi_connector_capability cap;
-};
-
struct ucsi {
struct device *dev;
struct ucsi_ppm *ppm;
@@ -238,8 +222,200 @@ static int ucsi_run_command(struct ucsi *ucsi, struct
ucsi_control *ctrl,
return ret;
}
+int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
+ void *retval, size_t size)
+{
+ int ret;
+
+ mutex_lock(&ucsi->ppm_lock);
+ ret = ucsi_run_command(ucsi, ctrl, retval, size);
+ mutex_unlock(&ucsi->ppm_lock);
+
+ return ret;
+}
+
/* --
*/
+void ucsi_altmode_update_active(struct ucsi_connector *con)
+{
+ const struct typec_altmode *altmode = NULL;
+ struct ucsi_control ctrl;
+ int ret;
+ u8 cur;
+
[PATCH 0/4] usb: typec: ucsi: Remaining changes for v5.2
Hi Greg, Here are the remaining patches from me and Ajay for the UCSI driver. I took the liberty of collecting them for you, and resending everything together. There are two patches from Ajay adding support for firmware upgrading with the Cypress CCGx controllers [1], and two patches from me enabling DisplayPort alt mode with the UCSI driver [2]. [1] https://marc.info/?l=linux-usb&m=154957412422108&w=2 [2] https://www.spinics.net/lists/linux-usb/msg178192.html Let us know if there if you want anything to be changed. thanks, -- heikki Ajay Gupta (2): usb: typec: ucsi: ccg: add get_fw_info function usb: typec: ucsi: ccg: add firmware flashing support Heikki Krogerus (2): usb: typec: ucsi: Preliminary support for alternate modes usb: typec: ucsi: Support for DisplayPort alt mode drivers/usb/typec/ucsi/Makefile | 15 +- drivers/usb/typec/ucsi/displayport.c | 297 + drivers/usb/typec/ucsi/trace.c | 12 + drivers/usb/typec/ucsi/trace.h | 26 + drivers/usb/typec/ucsi/ucsi.c| 371 +-- drivers/usb/typec/ucsi/ucsi.h| 93 +++ drivers/usb/typec/ucsi/ucsi_ccg.c| 884 ++- 7 files changed, 1617 insertions(+), 81 deletions(-) create mode 100644 drivers/usb/typec/ucsi/displayport.c -- 2.20.1
[PATCH 4/4] usb: typec: ucsi: Support for DisplayPort alt mode
This makes it possible to bind a driver to a DisplayPort
alt mode adapter devices.
The driver attempts to cope with the limitations of UCSI by
"emulating" behaviour and attempting to guess things when
ever possible in order to satisfy the requirements the
standard DisplayPort alt mode driver has.
Tested-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/Makefile | 15 +-
drivers/usb/typec/ucsi/displayport.c | 297 +++
drivers/usb/typec/ucsi/ucsi.c| 21 +-
drivers/usb/typec/ucsi/ucsi.h| 21 ++
4 files changed, 346 insertions(+), 8 deletions(-)
create mode 100644 drivers/usb/typec/ucsi/displayport.c
diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
index 2f4900b26210..b35e15a1f02c 100644
--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile
@@ -1,12 +1,15 @@
# SPDX-License-Identifier: GPL-2.0
-CFLAGS_trace.o := -I$(src)
+CFLAGS_trace.o := -I$(src)
-obj-$(CONFIG_TYPEC_UCSI) += typec_ucsi.o
+obj-$(CONFIG_TYPEC_UCSI) += typec_ucsi.o
-typec_ucsi-y := ucsi.o
+typec_ucsi-y := ucsi.o
-typec_ucsi-$(CONFIG_TRACING) += trace.o
+typec_ucsi-$(CONFIG_TRACING) += trace.o
-obj-$(CONFIG_UCSI_ACPI)+= ucsi_acpi.o
+ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
+ typec_ucsi-y+= displayport.o
+endif
-obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
+obj-$(CONFIG_UCSI_ACPI)+= ucsi_acpi.o
+obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
diff --git a/drivers/usb/typec/ucsi/displayport.c
b/drivers/usb/typec/ucsi/displayport.c
new file mode 100644
index ..2964103a6fc6
--- /dev/null
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UCSI DisplayPort Alternate Mode Support
+ *
+ * Copyright (C) 2018, Intel Corporation
+ * Author: Heikki Krogerus
+ */
+
+#include
+#include
+
+#include "ucsi.h"
+
+#define UCSI_CMD_SET_NEW_CAM(_con_num_, _enter_, _cam_, _am_) \
+(UCSI_SET_NEW_CAM | ((_con_num_) << 16) | ((_enter_) << 23) | \
+ ((_cam_) << 24) | ((u64)(_am_) << 32))
+
+struct ucsi_dp {
+ struct typec_displayport_data data;
+ struct ucsi_connector *con;
+ struct typec_altmode *alt;
+ struct work_struct work;
+ int offset;
+
+ bool override;
+ bool initialized;
+
+ u32 header;
+ u32 *vdo_data;
+ u8 vdo_size;
+};
+
+/*
+ * Note. Alternate mode control is optional feature in UCSI. It means that even
+ * if the system supports alternate modes, the OS may not be aware of them.
+ *
+ * In most cases however, the OS will be able to see the supported alternate
+ * modes, but it may still not be able to configure them, not even enter or
exit
+ * them. That is because UCSI defines alt mode details and alt mode
"overriding"
+ * as separate options.
+ *
+ * In case alt mode details are supported, but overriding is not, the driver
+ * will still display the supported pin assignments and configuration, but any
+ * changes the user attempts to do will lead into failure with return value of
+ * -EOPNOTSUPP.
+ */
+
+static int ucsi_displayport_enter(struct typec_altmode *alt)
+{
+ struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
+
+ mutex_lock(&dp->con->lock);
+
+ if (!dp->override && dp->initialized) {
+ const struct typec_altmode *p = typec_altmode_get_partner(alt);
+
+ dev_warn(&p->dev,
+"firmware doesn't support alternate mode
overriding\n");
+ mutex_unlock(&dp->con->lock);
+ return -EOPNOTSUPP;
+ }
+
+ /*
+* We can't send the New CAM command yet to the PPM as it needs the
+* configuration value as well. Pretending that we have now entered the
+* mode, and letting the alt mode driver continue.
+*/
+
+ dp->header = VDO(USB_TYPEC_DP_SID, 1, CMD_ENTER_MODE);
+ dp->header |= VDO_OPOS(USB_TYPEC_DP_MODE);
+ dp->header |= VDO_CMDT(CMDT_RSP_ACK);
+
+ dp->vdo_data = NULL;
+ dp->vdo_size = 1;
+
+ schedule_work(&dp->work);
+
+ mutex_unlock(&dp->con->lock);
+
+ return 0;
+}
+
+static int ucsi_displayport_exit(struct typec_altmode *alt)
+{
+ struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
+ struct ucsi_control ctrl;
+ int ret = 0;
+
+ mutex_lock(&dp->con->lock);
+
+ if (!dp->override) {
+ const struct typec_altmode *p = typec_altmode_get_partner(alt);
+
+ dev_warn(&p->dev,
+"firmware doesn't support alternate mode
overriding\n");
+
[PATCH 1/4] usb: typec: ucsi: ccg: add get_fw_info function
From: Ajay Gupta
Function is to get the details of ccg firmware and device version.
It will be useful in debugging and also during firmware update.
Signed-off-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/ucsi_ccg.c | 66 ++-
1 file changed, 64 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
b/drivers/usb/typec/ucsi/ucsi_ccg.c
index de8a43bdff68..3884fb41c72e 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -17,15 +17,54 @@
#include
#include "ucsi.h"
+enum enum_fw_mode {
+ BOOT, /* bootloader */
+ FW1,/* FW partition-1 (contains secondary fw) */
+ FW2,/* FW partition-2 (contains primary fw) */
+ FW_INVALID,
+};
+
+struct ccg_dev_info {
+#define CCG_DEVINFO_FWMODE_SHIFT (0)
+#define CCG_DEVINFO_FWMODE_MASK (0x3 << CCG_DEVINFO_FWMODE_SHIFT)
+#define CCG_DEVINFO_PDPORTS_SHIFT (2)
+#define CCG_DEVINFO_PDPORTS_MASK (0x3 << CCG_DEVINFO_PDPORTS_SHIFT)
+ u8 mode;
+ u8 bl_mode;
+ __le16 silicon_id;
+ __le16 bl_last_row;
+} __packed;
+
+struct version_format {
+ __le16 build;
+ u8 patch;
+ u8 ver;
+#define CCG_VERSION_MIN_SHIFT (0)
+#define CCG_VERSION_MIN_MASK (0xf << CCG_VERSION_MIN_SHIFT)
+#define CCG_VERSION_MAJ_SHIFT (4)
+#define CCG_VERSION_MAJ_MASK (0xf << CCG_VERSION_MAJ_SHIFT)
+} __packed;
+
+struct version_info {
+ struct version_format base;
+ struct version_format app;
+};
+
struct ucsi_ccg {
struct device *dev;
struct ucsi *ucsi;
struct ucsi_ppm ppm;
struct i2c_client *client;
+ struct ccg_dev_info info;
+ /* version info for boot, primary and secondary */
+ struct version_info version[FW2 + 1];
};
-#define CCGX_RAB_INTR_REG 0x06
-#define CCGX_RAB_UCSI_CONTROL 0x39
+#define CCGX_RAB_DEVICE_MODE 0x
+#define CCGX_RAB_INTR_REG 0x0006
+#define CCGX_RAB_READ_ALL_VER 0x0010
+#define CCGX_RAB_READ_FW2_VER 0x0020
+#define CCGX_RAB_UCSI_CONTROL 0x0039
#define CCGX_RAB_UCSI_CONTROL_STARTBIT(0)
#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
@@ -220,6 +259,23 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
return IRQ_HANDLED;
}
+static int get_fw_info(struct ucsi_ccg *uc)
+{
+ int err;
+
+ err = ccg_read(uc, CCGX_RAB_READ_ALL_VER, (u8 *)(&uc->version),
+ sizeof(uc->version));
+ if (err < 0)
+ return err;
+
+ err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
+ sizeof(uc->info));
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
static int ucsi_ccg_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -248,6 +304,12 @@ static int ucsi_ccg_probe(struct i2c_client *client,
return status;
}
+ status = get_fw_info(uc);
+ if (status < 0) {
+ dev_err(uc->dev, "get_fw_info failed - %d\n", status);
+ return status;
+ }
+
status = devm_request_threaded_irq(dev, client->irq, NULL,
ccg_irq_handler,
IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
--
2.20.1
[PATCH 2/4] usb: typec: ucsi: ccg: add firmware flashing support
From: Ajay Gupta
CCGx has two copies of the firmware in addition to the bootloader.
If the device is running FW1, FW2 can be updated with the new version.
Dual firmware mode allows the CCG device to stay in a PD contract and
support USB PD and Type-C functionality while a firmware update is in
progress.
First we read the currently flashed firmware version of both
primary and secondary firmware and then compare it with
version of firmware file to determine if flashing is required.
Command framework is added to support sending commands to CCGx
controller. We wait for response after sending the command and then
read the response from RAB_RESPONSE register.
Below commands are supported,
- ENTER_FLASHING
- RESET
- PDPORT_ENABLE
- JUMP_TO_BOOT
- FLASH_ROW_RW
- VALIDATE_FW
Command specific mutex lock is also added to sync between driver
and user threads.
PD port number information is added which is required while sending
PD_PORT_ENABLE command
Signed-off-by: Ajay Gupta
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/ucsi_ccg.c | 828 +-
1 file changed, 815 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 3884fb41c72e..5508beec08e3 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -9,6 +9,7 @@
*/
#include
#include
+#include
#include
#include
#include
@@ -24,6 +25,73 @@ enum enum_fw_mode {
FW_INVALID,
};
+#define CCGX_RAB_DEVICE_MODE 0x
+#define CCGX_RAB_INTR_REG 0x0006
+#define DEV_INT BIT(0)
+#define PORT0_INT BIT(1)
+#define PORT1_INT BIT(2)
+#define UCSI_READ_INT BIT(7)
+#define CCGX_RAB_JUMP_TO_BOOT 0x0007
+#define TO_BOOT 'J'
+#define TO_ALT_FW 'A'
+#define CCGX_RAB_RESET_REQ 0x0008
+#define RESET_SIG 'R'
+#define CMD_RESET_I2C 0x0
+#define CMD_RESET_DEV 0x1
+#define CCGX_RAB_ENTER_FLASHING0x000A
+#define FLASH_ENTER_SIG 'P'
+#define CCGX_RAB_VALIDATE_FW 0x000B
+#define CCGX_RAB_FLASH_ROW_RW 0x000C
+#define FLASH_SIG 'F'
+#define FLASH_RD_CMD 0x0
+#define FLASH_WR_CMD 0x1
+#define FLASH_FWCT1_WR_CMD0x2
+#define FLASH_FWCT2_WR_CMD0x3
+#define FLASH_FWCT_SIG_WR_CMD 0x4
+#define CCGX_RAB_READ_ALL_VER 0x0010
+#define CCGX_RAB_READ_FW2_VER 0x0020
+#define CCGX_RAB_UCSI_CONTROL 0x0039
+#define CCGX_RAB_UCSI_CONTROL_STARTBIT(0)
+#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
+#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
+#define REG_FLASH_RW_MEM0x0200
+#define DEV_REG_IDXCCGX_RAB_DEVICE_MODE
+#define CCGX_RAB_PDPORT_ENABLE 0x002C
+#define PDPORT_1 BIT(0)
+#define PDPORT_2 BIT(1)
+#define CCGX_RAB_RESPONSE 0x007E
+#define ASYNC_EVENT BIT(7)
+
+/* CCGx events & async msg codes */
+#define RESET_COMPLETE 0x80
+#define EVENT_INDEXRESET_COMPLETE
+#define PORT_CONNECT_DET 0x84
+#define PORT_DISCONNECT_DET0x85
+#define ROLE_SWAP_COMPELETE0x87
+
+/* ccg firmware */
+#define CYACD_LINE_SIZE 527
+#define CCG4_ROW_SIZE 256
+#define FW1_METADATA_ROW0x1FF
+#define FW2_METADATA_ROW0x1FE
+#define FW_CFG_TABLE_SIG_SIZE 256
+
+static int secondary_fw_min_ver = 41;
+
+enum enum_flash_mode {
+ SECONDARY_BL, /* update secondary using bootloader */
+ PRIMARY,/* update primary using secondary */
+ SECONDARY, /* update secondary using primary */
+ FLASH_NOT_NEEDED, /* update not required */
+ FLASH_INVALID,
+};
+
+static const char * const ccg_fw_names[] = {
+ "ccg_boot.cyacd",
+ "ccg_primary.cyacd",
+ "ccg_secondary.cyacd"
+};
+
struct ccg_dev_info {
#define CCG_DEVINFO_FWMODE_SHIFT (0)
#define CCG_DEVINFO_FWMODE_MASK (0x3 << CCG_DEVINFO_FWMODE_SHIFT)
@@ -50,6 +118,50 @@ struct version_info {
struct version_format app;
};
+struct fw_config_table {
+ u32 identity;
+ u16 table_size;
+ u8 fwct_version;
+ u8 is_key_change;
+ u8 guid[16];
+ struct version_format base;
+ struct version_format app;
+ u8 primary_fw_digest[32];
+ u32 key_exp_leng
Re: [PATCH] usb: typec: Registering real device entries for the muxes
On Tue, Apr 02, 2019 at 03:47:47PM +0300, Heikki Krogerus wrote:
> On Mon, Apr 01, 2019 at 02:45:11PM +0200, Hans de Goede wrote:
> > HI,
> >
> > On 01-04-19 14:40, Heikki Krogerus wrote:
> > > On Mon, Apr 01, 2019 at 12:34:29PM +0200, Greg KH wrote:
> > > > On Mon, Apr 01, 2019 at 01:15:53PM +0300, Heikki Krogerus wrote:
> > > > > Registering real device entries (struct device) for the mode
> > > > > muxes as well as for the orientation switches.
> > > > >
> > > > > The Type-C mux code was deliberately attempting to avoid
> > > > > creation of separate device entries for the orientation
> > > > > switch and the mode switch (alternate modes) because they
> > > > > are not physical devices. They are functions of a single
> > > > > physical multiplexer/demultiplexer switch device.
> > > > >
> > > > > Unfortunately because of the dependency we still have on the
> > > > > underlying mux device driver, we had to put in hacks like
> > > > > the one in the commit 3e3b81965cbf ("usb: typec: mux: Take
> > > > > care of driver module reference counting") to make sure the
> > > > > driver does not disappear from underneath us. Even with
> > > > > those hacks we were still left with a potential NUll pointer
> > > > > dereference scenario, so just creating the device entries,
> > > > > and letting the core take care of the dependencies. No more
> > > > > hacks needed.
> > > > >
> > > > > Fixes: 3e3b81965cbf ("usb: typec: mux: Take care of driver module
> > > > > reference counting")
> > > > > Cc: v4.19.x # v4.19.x+
> > > > > Signed-off-by: Heikki Krogerus
> > > >
> > > > This looks good to me, nice work!
> > > >
> > > > But, it would be nice if someone who has this hardware can test it to
> > > > verify it does actually work :)
> > >
> > > This alone does not work on Intel Cherrytrail platforms. I need to make
> > > the Intel Cherrytrail MFD driver (intel_cht_int33fe.c) to use the new
> > > device names that we now have for the muxes. Sorry for the mistake.
> > >
> > > I'll resend this and include the needed modifications to
> > > intel_cht_int33fe.c. Hans should be able to test this once I do that. I
> > > hope he has time.
> >
> > Yes I need to get back to the CherryTrail Type-C stuff we discussed a while
> > back anyways. On which tree/branch should I test v2 of this patch(series) ?
>
> Greg's usb-next.
I just realized that the potential NULL pointer dereference that this
patch is meant to fix can't happen until my "software node reference"
modifications [1] have been applied. I don't think this needs to be
considered a fix.
I think the correct way to handle this patch is to make just it part
of that series. I'll resend the series, and include this patch in it.
[1] https://lkml.org/lkml/2019/3/15/457
thanks,
--
heikki
Re: [PATCH] usb: typec: Registering real device entries for the muxes
On Mon, Apr 01, 2019 at 02:45:11PM +0200, Hans de Goede wrote:
> HI,
>
> On 01-04-19 14:40, Heikki Krogerus wrote:
> > On Mon, Apr 01, 2019 at 12:34:29PM +0200, Greg KH wrote:
> > > On Mon, Apr 01, 2019 at 01:15:53PM +0300, Heikki Krogerus wrote:
> > > > Registering real device entries (struct device) for the mode
> > > > muxes as well as for the orientation switches.
> > > >
> > > > The Type-C mux code was deliberately attempting to avoid
> > > > creation of separate device entries for the orientation
> > > > switch and the mode switch (alternate modes) because they
> > > > are not physical devices. They are functions of a single
> > > > physical multiplexer/demultiplexer switch device.
> > > >
> > > > Unfortunately because of the dependency we still have on the
> > > > underlying mux device driver, we had to put in hacks like
> > > > the one in the commit 3e3b81965cbf ("usb: typec: mux: Take
> > > > care of driver module reference counting") to make sure the
> > > > driver does not disappear from underneath us. Even with
> > > > those hacks we were still left with a potential NUll pointer
> > > > dereference scenario, so just creating the device entries,
> > > > and letting the core take care of the dependencies. No more
> > > > hacks needed.
> > > >
> > > > Fixes: 3e3b81965cbf ("usb: typec: mux: Take care of driver module
> > > > reference counting")
> > > > Cc: v4.19.x # v4.19.x+
> > > > Signed-off-by: Heikki Krogerus
> > >
> > > This looks good to me, nice work!
> > >
> > > But, it would be nice if someone who has this hardware can test it to
> > > verify it does actually work :)
> >
> > This alone does not work on Intel Cherrytrail platforms. I need to make
> > the Intel Cherrytrail MFD driver (intel_cht_int33fe.c) to use the new
> > device names that we now have for the muxes. Sorry for the mistake.
> >
> > I'll resend this and include the needed modifications to
> > intel_cht_int33fe.c. Hans should be able to test this once I do that. I
> > hope he has time.
>
> Yes I need to get back to the CherryTrail Type-C stuff we discussed a while
> back anyways. On which tree/branch should I test v2 of this patch(series) ?
Greg's usb-next.
thanks,
--
heikki
Re: [PATCH] usb: typec: Registering real device entries for the muxes
On Mon, Apr 01, 2019 at 12:34:29PM +0200, Greg KH wrote:
> On Mon, Apr 01, 2019 at 01:15:53PM +0300, Heikki Krogerus wrote:
> > Registering real device entries (struct device) for the mode
> > muxes as well as for the orientation switches.
> >
> > The Type-C mux code was deliberately attempting to avoid
> > creation of separate device entries for the orientation
> > switch and the mode switch (alternate modes) because they
> > are not physical devices. They are functions of a single
> > physical multiplexer/demultiplexer switch device.
> >
> > Unfortunately because of the dependency we still have on the
> > underlying mux device driver, we had to put in hacks like
> > the one in the commit 3e3b81965cbf ("usb: typec: mux: Take
> > care of driver module reference counting") to make sure the
> > driver does not disappear from underneath us. Even with
> > those hacks we were still left with a potential NUll pointer
> > dereference scenario, so just creating the device entries,
> > and letting the core take care of the dependencies. No more
> > hacks needed.
> >
> > Fixes: 3e3b81965cbf ("usb: typec: mux: Take care of driver module reference
> > counting")
> > Cc: v4.19.x # v4.19.x+
> > Signed-off-by: Heikki Krogerus
>
> This looks good to me, nice work!
>
> But, it would be nice if someone who has this hardware can test it to
> verify it does actually work :)
This alone does not work on Intel Cherrytrail platforms. I need to make
the Intel Cherrytrail MFD driver (intel_cht_int33fe.c) to use the new
device names that we now have for the muxes. Sorry for the mistake.
I'll resend this and include the needed modifications to
intel_cht_int33fe.c. Hans should be able to test this once I do that. I
hope he has time.
thanks,
--
heikki
[PATCH] usb: typec: Registering real device entries for the muxes
Registering real device entries (struct device) for the mode
muxes as well as for the orientation switches.
The Type-C mux code was deliberately attempting to avoid
creation of separate device entries for the orientation
switch and the mode switch (alternate modes) because they
are not physical devices. They are functions of a single
physical multiplexer/demultiplexer switch device.
Unfortunately because of the dependency we still have on the
underlying mux device driver, we had to put in hacks like
the one in the commit 3e3b81965cbf ("usb: typec: mux: Take
care of driver module reference counting") to make sure the
driver does not disappear from underneath us. Even with
those hacks we were still left with a potential NUll pointer
dereference scenario, so just creating the device entries,
and letting the core take care of the dependencies. No more
hacks needed.
Fixes: 3e3b81965cbf ("usb: typec: mux: Take care of driver module reference
counting")
Cc: v4.19.x # v4.19.x+
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/bus.h | 15 ++
drivers/usb/typec/class.c | 16 ++-
drivers/usb/typec/mux.c | 207 +++-
drivers/usb/typec/mux/pi3usb30532.c | 46 ---
include/linux/usb/typec_mux.h | 58
5 files changed, 220 insertions(+), 122 deletions(-)
diff --git a/drivers/usb/typec/bus.h b/drivers/usb/typec/bus.h
index db40e61d8b72..0c9661c96473 100644
--- a/drivers/usb/typec/bus.h
+++ b/drivers/usb/typec/bus.h
@@ -35,4 +35,19 @@ extern const struct device_type typec_port_dev_type;
#define is_typec_altmode(_dev_) (_dev_->type == &typec_altmode_dev_type)
#define is_typec_port(_dev_) (_dev_->type == &typec_port_dev_type)
+extern struct class typec_mux_class;
+
+struct typec_switch {
+ struct device dev;
+ typec_switch_set_fn_t set;
+};
+
+struct typec_mux {
+ struct device dev;
+ typec_mux_set_fn_t set;
+};
+
+#define to_typec_switch(_dev_) container_of(_dev_, struct typec_switch, dev)
+#define to_typec_mux(_dev_) container_of(_dev_, struct typec_mux, dev)
+
#endif /* __USB_TYPEC_ALTMODE_H__ */
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 2eb623841847..8f5223879d91 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1646,13 +1646,25 @@ static int __init typec_init(void)
if (ret)
return ret;
+ ret = class_register(&typec_mux_class);
+ if (ret)
+ goto err_unregister_bus;
+
typec_class = class_create(THIS_MODULE, "typec");
if (IS_ERR(typec_class)) {
- bus_unregister(&typec_bus);
- return PTR_ERR(typec_class);
+ ret = PTR_ERR(typec_class);
+ goto err_unregister_mux_class;
}
return 0;
+
+err_unregister_mux_class:
+ class_unregister(&typec_mux_class);
+
+err_unregister_bus:
+ bus_unregister(&typec_bus);
+
+ return ret;
}
subsys_initcall(typec_init);
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index 2ce54f3fc79c..f945abd23222 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -15,35 +15,35 @@
#include
#include
-static DEFINE_MUTEX(switch_lock);
-static DEFINE_MUTEX(mux_lock);
-static LIST_HEAD(switch_list);
-static LIST_HEAD(mux_list);
+#include "bus.h"
+
+static int name_match(struct device *dev, const void *name)
+{
+ return !strcmp((const char *)name, dev_name(dev));
+}
+
+static int fwnode_match(struct device *dev, const void *fwnode)
+{
+ return dev_fwnode(dev) == fwnode;
+}
static void *typec_switch_match(struct device_connection *con, int ep,
void *data)
{
- struct typec_switch *sw;
+ struct device *dev;
- if (!con->fwnode) {
- list_for_each_entry(sw, &switch_list, entry)
- if (!strcmp(con->endpoint[ep], dev_name(sw->dev)))
- return sw;
- return ERR_PTR(-EPROBE_DEFER);
- }
-
- /*
-* With OF graph the mux node must have a boolean device property named
-* "orientation-switch".
-*/
- if (con->id && !fwnode_property_present(con->fwnode, con->id))
- return NULL;
+ if (con->fwnode) {
+ if (con->id && !fwnode_property_present(con->fwnode, con->id))
+ return NULL;
- list_for_each_entry(sw, &switch_list, entry)
- if (dev_fwnode(sw->dev) == con->fwnode)
- return sw;
+ dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
+ fwnode_match);
+ } else {
+ dev = class_find_device(&typec_mux_class, NULL,
+
Re: [PATCH] usb: typec: mux: Store module handle
On Fri, Mar 29, 2019 at 02:17:01PM +, Sasha Levin wrote:
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: .
>
> The bot has tested the following trees: v5.0.5, v4.19.32, v4.14.109,
> v4.9.166, v4.4.177, v3.18.137.
>
> v5.0.5: Build OK!
> v4.19.32: Build OK!
> v4.14.109: Failed to apply! Possible dependencies:
> 3e3b81965cbf ("usb: typec: mux: Take care of driver module reference
> counting")
> 44262fad12a7 ("staging: typec: tcpm: Drop commented out code")
> 4b4e02c83167 ("typec: tcpm: Move out of staging")
> 5fd54ace4721 ("USB: add SPDX identifiers to all remaining files in
> drivers/usb/")
> 70cd90be3300 ("staging: typec: pd: Document struct pd_message")
> 98076fa64a05 ("staging: typec: tcpm: Document data structures")
> bdecb33af34f ("usb: typec: API for controlling USB Type-C Multiplexers")
>
> v4.9.166: Failed to apply! Possible dependencies:
> 02d5be466b68 ("staging: typec: tcpm: Prevent TCPM from looping in
> SRC_TRYWAIT")
> 050161ea3268 ("staging: typec: tcpm: Fix Port Power Role field in PS_RDY
> messages")
> 131c7d12ef21 ("staging: typec: tcpm: Follow Try.SRC exit requirements")
> 193a68011fdc ("staging: typec: tcpm: Respond to Discover Identity
> commands")
> 23b028c871e1 ("staging: bcm2835-audio: initial staging submission")
> 32774ef3e4bb ("staging: vc04_services: use bcm2835 consequently")
> 3e3b81965cbf ("usb: typec: mux: Take care of driver module reference
> counting")
> 4b4e02c83167 ("typec: tcpm: Move out of staging")
> 5383fae76b82 ("doc-rst: fixed kernel-doc directives in usb/typec.rst")
> 5fec4b54d0bf ("staging: typec: tcpm: Drop duplicate PD messages")
> 70cd90be3300 ("staging: typec: pd: Document struct pd_message")
> 74e656d6b055 ("staging: typec: Type-C Port Controller Interface driver
> (tcpci)")
> 9b0ae69909c2 ("staging: typec: tcpm: set port type callback")
> a0a3e04e6b2c ("staging: typec: tcpm: Check for Rp for tPDDebounce")
> b17dd57118fe ("staging: typec: tcpm: Improve role swap with non PD
> capable partners")
> b965b6317ff1 ("staging: typec: tcpm: typec: tcpm: Wait for CC debounce
> before PD excg")
> bdecb33af34f ("usb: typec: API for controlling USB Type-C Multiplexers")
> c034a43e72dd ("staging: typec: Fairchild FUSB302 Type-c chip driver")
> c749d4d0e4b8 ("staging: typec: tcpm: Set default state after error
> recovery based on port type")
> c79d92bda80c ("staging: typec: tcpm: Check cc status before entering
> SRC_TRY_DEBOUCE")
> f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")
> fab9288428ec ("usb: USB Type-C connector class")
> fce042f02ef0 ("staging: typec: tcpm: Report right typec_pwr_opmode")
>
> v4.4.177: Failed to apply! Possible dependencies:
> 13a9930d15b4 ("staging: ks7010: add driver from Nanonote
> extra-repository")
> 199d68d4a8cb ("greybus: start moving the function types into the greybus
> core")
> 23b028c871e1 ("staging: bcm2835-audio: initial staging submission")
> 27fb83109a39 ("greybus: register the bus with the driver core and add
> framework for debugfs files.")
> 3e3b81965cbf ("usb: typec: mux: Take care of driver module reference
> counting")
> 4b4e02c83167 ("typec: tcpm: Move out of staging")
> 53419e07cc94 ("greybus: i2c-gb: actually add the i2c adapter properly...")
> 5383fae76b82 ("doc-rst: fixed kernel-doc directives in usb/typec.rst")
> 71bad7f08641 ("staging: add bcm2708 vchiq driver")
> 79c822be7b85 ("greybus: uart framework added, doesn't build")
> 83ddaaab01c2 ("greybus: Greybus SD/MMC host driver")
> 85f37d17b3f1 ("isdn: act200: fix MODULE_PARM_DESC() typo")
> a18e15175708 ("greybus: more uart work")
> a921e9bd4e22 ("isdn: i4l: move active-isdn drivers to staging")
> bdecb33af34f ("usb: typec: API for controlling USB Type-C Multiplexers")
> c16854c3bf56 ("greybus: gpio driver")
> c8a797a98cb6 ("greybus: Import most recent greybus code to new repo.")
> d4f56b47a8fa ("staging: greybus: Add drivers/staging/greybus to the
> build")
> d5d1903dcd15 ("greybus: add framework for 'struct gbuf'")
> db6e1fd264ac ("greybus: hook up sdio, gpio, and tty into the greybus
> core.")
> de536e309476 ("greybus: ap message loop added.")
> e68453ed28c5 ("greybus: uart-gb: now builds, more framework added")
> e9023d227ab8 ("greybus: gpio-gb.c: it now builds properly")
> f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")
> fab9288428ec ("usb: USB Type-C connector class")
> ff45c265f849 ("greybus: uart-gb: more work on tty functions")
>
> v3.18.137: Failed to apply! Possible dependencies:
> 01ed1e1504ac ("isdn: icn: remove a #warning")
> 10640d34552c ("isdn: icn: use strlcpy() when parsing setup options")
> 13a9930d15b4 ("staging: ks7010: add driver from Nanonote
> extra-repository")
> 23b028
Re: [PATCH] usb: typec: mux: Store module handle
On Thu, Mar 28, 2019 at 01:02:50PM +0100, Greg KH wrote:
> On Thu, Mar 28, 2019 at 02:52:08PM +0300, Heikki Krogerus wrote:
> > It is possible that the driver of the mux device has been
> > unbind by the time typec_mux_put() or typec_switch_put() is
> > called.
> >
> > To prevent the NULL Pointer Dereference from happening in
> > this case when decrementing the reference count of the
> > module by using dev->driver->owner, storing the module
> > handle to the mux and switch data structures, and using the
> > stored value instead.
> >
> > Fixes: ("3e3b81965cbf usb: typec: mux: Take care of driver module reference
> > counting")
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Heikki Krogerus
> > ---
> > drivers/usb/typec/mux.c | 10 ++
> > include/linux/usb/typec_mux.h | 2 ++
> > 2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > --- a/include/linux/usb/typec_mux.h
> > +++ b/include/linux/usb/typec_mux.h
> > @@ -20,6 +20,7 @@ struct device;
> > */
> > struct typec_switch {
> > struct device *dev;
> > + struct module *module;
> > struct list_head entry;
> >
> > int (*set)(struct typec_switch *sw, enum typec_orientation orientation);
> > @@ -37,6 +38,7 @@ struct typec_switch {
> > */
> > struct typec_mux {
> > struct device *dev;
> > + struct module *module;
> > struct list_head entry;
>
> You have just created two different reference counts for a single object
> here :(
>
> This is data, not code. Data needs to be protected with the reference
> count to keep it from being freed while in use. Code also needs to be
> protected, BUT, do not mix the two.
>
> driver owners should always be properly reference counted if userspace
> opens the device, not if another internal kernel code opens the device.
> Or better yet, just properly unwind things when the code is removed, no
> need to reference count anything on the module level (like networking
> devices do it).
>
> So I really do not think this patch is correct, and odds are, the
> original one that this patch says it fixes is probably also not correct
> :(
OK. I'll see if I can prepare a proper fix for the original.
thanks,
--
heikki
[PATCH] usb: typec: mux: Store module handle
It is possible that the driver of the mux device has been
unbind by the time typec_mux_put() or typec_switch_put() is
called.
To prevent the NULL Pointer Dereference from happening in
this case when decrementing the reference count of the
module by using dev->driver->owner, storing the module
handle to the mux and switch data structures, and using the
stored value instead.
Fixes: ("3e3b81965cbf usb: typec: mux: Take care of driver module reference
counting")
Cc: sta...@vger.kernel.org
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/mux.c | 10 ++
include/linux/usb/typec_mux.h | 2 ++
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index 2ce54f3fc79c..617687c4eb20 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -63,7 +63,8 @@ struct typec_switch *typec_switch_get(struct device *dev)
sw = device_connection_find_match(dev, "orientation-switch", NULL,
typec_switch_match);
if (!IS_ERR_OR_NULL(sw)) {
- WARN_ON(!try_module_get(sw->dev->driver->owner));
+ sw->module = sw->dev->driver->owner;
+ WARN_ON(!try_module_get(sw->module));
get_device(sw->dev);
}
mutex_unlock(&switch_lock);
@@ -81,7 +82,7 @@ EXPORT_SYMBOL_GPL(typec_switch_get);
void typec_switch_put(struct typec_switch *sw)
{
if (!IS_ERR_OR_NULL(sw)) {
- module_put(sw->dev->driver->owner);
+ module_put(sw->module);
put_device(sw->dev);
}
}
@@ -206,7 +207,8 @@ struct typec_mux *typec_mux_get(struct device *dev,
mux = device_connection_find_match(dev, "mode-switch", (void *)desc,
typec_mux_match);
if (!IS_ERR_OR_NULL(mux)) {
- WARN_ON(!try_module_get(mux->dev->driver->owner));
+ mux->module = mux->dev->driver->owner;
+ WARN_ON(!try_module_get(mux->module));
get_device(mux->dev);
}
mutex_unlock(&mux_lock);
@@ -224,7 +226,7 @@ EXPORT_SYMBOL_GPL(typec_mux_get);
void typec_mux_put(struct typec_mux *mux)
{
if (!IS_ERR_OR_NULL(mux)) {
- module_put(mux->dev->driver->owner);
+ module_put(mux->module);
put_device(mux->dev);
}
}
diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
index 43f40685e53c..b31498020bf3 100644
--- a/include/linux/usb/typec_mux.h
+++ b/include/linux/usb/typec_mux.h
@@ -20,6 +20,7 @@ struct device;
*/
struct typec_switch {
struct device *dev;
+ struct module *module;
struct list_head entry;
int (*set)(struct typec_switch *sw, enum typec_orientation orientation);
@@ -37,6 +38,7 @@ struct typec_switch {
*/
struct typec_mux {
struct device *dev;
+ struct module *module;
struct list_head entry;
int (*set)(struct typec_mux *mux, int state);
--
2.20.1
Re: [PATCH v2 0/2] usb: typec: ucsi: Support for DP alt mode
On Mon, Mar 25, 2019 at 10:16:36PM +, Ajay Gupta wrote: > Hi Heikki, > > > -Original Message- > > From: linux-usb-ow...@vger.kernel.org On > > Behalf Of Heikki Krogerus > > Sent: Wednesday, March 20, 2019 4:45 AM > > To: Ajay Gupta ; Michael Hsu > > Cc: linux-usb@vger.kernel.org > > Subject: [PATCH v2 0/2] usb: typec: ucsi: Support for DP alt mode > > > > Hi, > > > > This is the second version of my proposal to add alt mode handling to ucsi > > driver. > > The problem in the first version was that I was not using the port alt mode > > that > > was associated with the partner alt mode (and vise versa), but I was instead > > searching the alt mode objects in every case separately. > > > > So in this version I'm now always using the already associated alternate > > mode > > partner in every case, and as far as the UCSI driver (or any other USB > > Type-C > > driver) is concerned, that is all that it needs to do. The problem of > > actually > > associating the correct alt mode partner object belongs to the Type-C > > subsystem > > core code, and any improvements to that code are out side of the scope of > > this > > series. > > > > I've prepared a patch that allows the alt mode drivers to find the correct > > port alt > > mode for the partner alt modes. That should cover SVIDs that define multiple > > modes (note, DisplayPort Alt mode is not one of those). But as I mentioned > > above, that is not related to this series. > Thanks for the new set. It looks good to me. > I tested it with UCSI CCG on NVIDIA GPU and it works. > > Tested-By: Ajay Gupta Thank you! Br, -- heikki
[PATCH v2 1/2] usb: typec: ucsi: Preliminary support for alternate modes
With UCSI the alternate modes, just like everything else
related to USB Type-C connectors, are handled in firmware.
The operating system can see the status and is allowed to
request certain things, for example entering and exiting the
modes, but the support for alternate modes is very limited
in UCSI. The feature is also optional, which means that even
when the platform supports alternate modes, the operating
system may not be even made aware of them.
UCSI does not support direct VDM reading or writing.
Instead, alternate modes can be entered and exited using a
single custom command which takes also an optional SVID
specific configuration value as parameter. That means every
supported alternate mode has to be handled separately in
UCSI driver.
This commit does not include support for any specific
alternate mode. The discovered alternate modes are now
registered, but binding a driver to an alternate mode will
not be possible until support for that alternate mode is
added to the UCSI driver.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/trace.c | 12 ++
drivers/usb/typec/ucsi/trace.h | 26 +++
drivers/usb/typec/ucsi/ucsi.c | 349 +++--
drivers/usb/typec/ucsi/ucsi.h | 72 +++
4 files changed, 394 insertions(+), 65 deletions(-)
diff --git a/drivers/usb/typec/ucsi/trace.c b/drivers/usb/typec/ucsi/trace.c
index ffa3b4c3f338..1dabafb74320 100644
--- a/drivers/usb/typec/ucsi/trace.c
+++ b/drivers/usb/typec/ucsi/trace.c
@@ -60,3 +60,15 @@ const char *ucsi_cci_str(u32 cci)
return "";
}
+
+static const char * const ucsi_recipient_strs[] = {
+ [UCSI_RECIPIENT_CON]= "port",
+ [UCSI_RECIPIENT_SOP]= "partner",
+ [UCSI_RECIPIENT_SOP_P] = "plug (prime)",
+ [UCSI_RECIPIENT_SOP_PP] = "plug (double prime)",
+};
+
+const char *ucsi_recipient_str(u8 recipient)
+{
+ return ucsi_recipient_strs[recipient];
+}
diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h
index 5e2906df2db7..783ec9c72055 100644
--- a/drivers/usb/typec/ucsi/trace.h
+++ b/drivers/usb/typec/ucsi/trace.h
@@ -7,6 +7,7 @@
#define __UCSI_TRACE_H
#include
+#include
const char *ucsi_cmd_str(u64 raw_cmd);
const char *ucsi_ack_str(u8 ack);
@@ -134,6 +135,31 @@ DEFINE_EVENT(ucsi_log_connector_status, ucsi_register_port,
TP_ARGS(port, status)
);
+DECLARE_EVENT_CLASS(ucsi_log_register_altmode,
+ TP_PROTO(u8 recipient, struct typec_altmode *alt),
+ TP_ARGS(recipient, alt),
+ TP_STRUCT__entry(
+ __field(u8, recipient)
+ __field(u16, svid)
+ __field(u8, mode)
+ __field(u32, vdo)
+ ),
+ TP_fast_assign(
+ __entry->recipient = recipient;
+ __entry->svid = alt->svid;
+ __entry->mode = alt->mode;
+ __entry->vdo = alt->vdo;
+ ),
+ TP_printk("%s alt mode: svid %04x, mode %d vdo %x",
+ ucsi_recipient_str(__entry->recipient), __entry->svid,
+ __entry->mode, __entry->vdo)
+);
+
+DEFINE_EVENT(ucsi_log_register_altmode, ucsi_register_altmode,
+ TP_PROTO(u8 recipient, struct typec_altmode *alt),
+ TP_ARGS(recipient, alt)
+);
+
#endif /* __UCSI_TRACE_H */
/* This part must be outside protection */
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 8d0a6fe748bd..39f256af84b1 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -12,7 +12,7 @@
#include
#include
#include
-#include
+#include
#include "ucsi.h"
#include "trace.h"
@@ -45,22 +45,6 @@ enum ucsi_status {
UCSI_ERROR,
};
-struct ucsi_connector {
- int num;
-
- struct ucsi *ucsi;
- struct work_struct work;
- struct completion complete;
-
- struct typec_port *port;
- struct typec_partner *partner;
-
- struct typec_capability typec_cap;
-
- struct ucsi_connector_status status;
- struct ucsi_connector_capability cap;
-};
-
struct ucsi {
struct device *dev;
struct ucsi_ppm *ppm;
@@ -238,8 +222,199 @@ static int ucsi_run_command(struct ucsi *ucsi, struct
ucsi_control *ctrl,
return ret;
}
+int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
+ void *retval, size_t size)
+{
+ int ret;
+
+ mutex_lock(&ucsi->ppm_lock);
+ ret = ucsi_run_command(ucsi, ctrl, retval, size);
+ mutex_unlock(&ucsi->ppm_lock);
+
+ return ret;
+}
+
/* --
*/
+void ucsi_altmode_update_active(struct ucsi_connector *con)
+{
+ const struct typec_altmode *altmode;
+ struct ucsi_control ctrl;
+ int ret;
+ u8 cur;
+ int i;
+
+ UCS
[PATCH v2 2/2] usb: typec: ucsi: Support for DisplayPort alt mode
This makes it possible to bind a driver to a DisplayPort
alt mode adapter devices.
The driver attempts to cope with the limitations of UCSI by
"emulating" behaviour and attempting to guess things when
ever possible in order to satisfy the requirements the
standard DisplayPort alt mode driver has.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/ucsi/Makefile | 15 +-
drivers/usb/typec/ucsi/displayport.c | 297 +++
drivers/usb/typec/ucsi/ucsi.c| 21 +-
drivers/usb/typec/ucsi/ucsi.h| 21 ++
4 files changed, 346 insertions(+), 8 deletions(-)
create mode 100644 drivers/usb/typec/ucsi/displayport.c
diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
index 2f4900b26210..b35e15a1f02c 100644
--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile
@@ -1,12 +1,15 @@
# SPDX-License-Identifier: GPL-2.0
-CFLAGS_trace.o := -I$(src)
+CFLAGS_trace.o := -I$(src)
-obj-$(CONFIG_TYPEC_UCSI) += typec_ucsi.o
+obj-$(CONFIG_TYPEC_UCSI) += typec_ucsi.o
-typec_ucsi-y := ucsi.o
+typec_ucsi-y := ucsi.o
-typec_ucsi-$(CONFIG_TRACING) += trace.o
+typec_ucsi-$(CONFIG_TRACING) += trace.o
-obj-$(CONFIG_UCSI_ACPI)+= ucsi_acpi.o
+ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
+ typec_ucsi-y+= displayport.o
+endif
-obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
+obj-$(CONFIG_UCSI_ACPI)+= ucsi_acpi.o
+obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
diff --git a/drivers/usb/typec/ucsi/displayport.c
b/drivers/usb/typec/ucsi/displayport.c
new file mode 100644
index ..2964103a6fc6
--- /dev/null
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UCSI DisplayPort Alternate Mode Support
+ *
+ * Copyright (C) 2018, Intel Corporation
+ * Author: Heikki Krogerus
+ */
+
+#include
+#include
+
+#include "ucsi.h"
+
+#define UCSI_CMD_SET_NEW_CAM(_con_num_, _enter_, _cam_, _am_) \
+(UCSI_SET_NEW_CAM | ((_con_num_) << 16) | ((_enter_) << 23) | \
+ ((_cam_) << 24) | ((u64)(_am_) << 32))
+
+struct ucsi_dp {
+ struct typec_displayport_data data;
+ struct ucsi_connector *con;
+ struct typec_altmode *alt;
+ struct work_struct work;
+ int offset;
+
+ bool override;
+ bool initialized;
+
+ u32 header;
+ u32 *vdo_data;
+ u8 vdo_size;
+};
+
+/*
+ * Note. Alternate mode control is optional feature in UCSI. It means that even
+ * if the system supports alternate modes, the OS may not be aware of them.
+ *
+ * In most cases however, the OS will be able to see the supported alternate
+ * modes, but it may still not be able to configure them, not even enter or
exit
+ * them. That is because UCSI defines alt mode details and alt mode
"overriding"
+ * as separate options.
+ *
+ * In case alt mode details are supported, but overriding is not, the driver
+ * will still display the supported pin assignments and configuration, but any
+ * changes the user attempts to do will lead into failure with return value of
+ * -EOPNOTSUPP.
+ */
+
+static int ucsi_displayport_enter(struct typec_altmode *alt)
+{
+ struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
+
+ mutex_lock(&dp->con->lock);
+
+ if (!dp->override && dp->initialized) {
+ const struct typec_altmode *p = typec_altmode_get_partner(alt);
+
+ dev_warn(&p->dev,
+"firmware doesn't support alternate mode
overriding\n");
+ mutex_unlock(&dp->con->lock);
+ return -EOPNOTSUPP;
+ }
+
+ /*
+* We can't send the New CAM command yet to the PPM as it needs the
+* configuration value as well. Pretending that we have now entered the
+* mode, and letting the alt mode driver continue.
+*/
+
+ dp->header = VDO(USB_TYPEC_DP_SID, 1, CMD_ENTER_MODE);
+ dp->header |= VDO_OPOS(USB_TYPEC_DP_MODE);
+ dp->header |= VDO_CMDT(CMDT_RSP_ACK);
+
+ dp->vdo_data = NULL;
+ dp->vdo_size = 1;
+
+ schedule_work(&dp->work);
+
+ mutex_unlock(&dp->con->lock);
+
+ return 0;
+}
+
+static int ucsi_displayport_exit(struct typec_altmode *alt)
+{
+ struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
+ struct ucsi_control ctrl;
+ int ret = 0;
+
+ mutex_lock(&dp->con->lock);
+
+ if (!dp->override) {
+ const struct typec_altmode *p = typec_altmode_get_partner(alt);
+
+ dev_warn(&p->dev,
+"firmware doesn't support alternate mode
overriding\n");
+ ret = -EOPNOTSUPP;
+
[PATCH v2 0/2] usb: typec: ucsi: Support for DP alt mode
Hi, This is the second version of my proposal to add alt mode handling to ucsi driver. The problem in the first version was that I was not using the port alt mode that was associated with the partner alt mode (and vise versa), but I was instead searching the alt mode objects in every case separately. So in this version I'm now always using the already associated alternate mode partner in every case, and as far as the UCSI driver (or any other USB Type-C driver) is concerned, that is all that it needs to do. The problem of actually associating the correct alt mode partner object belongs to the Type-C subsystem core code, and any improvements to that code are out side of the scope of this series. I've prepared a patch that allows the alt mode drivers to find the correct port alt mode for the partner alt modes. That should cover SVIDs that define multiple modes (note, DisplayPort Alt mode is not one of those). But as I mentioned above, that is not related to this series. You can check the v1 from here: https://www.spinics.net/lists/linux-usb/msg176703.html thanks, Heikki Krogerus (2): usb: typec: ucsi: Preliminary support for alternate modes usb: typec: ucsi: Support for DisplayPort alt mode drivers/usb/typec/ucsi/Makefile | 15 +- drivers/usb/typec/ucsi/displayport.c | 297 ++ drivers/usb/typec/ucsi/trace.c | 12 + drivers/usb/typec/ucsi/trace.h | 26 ++ drivers/usb/typec/ucsi/ucsi.c| 366 ++- drivers/usb/typec/ucsi/ucsi.h| 93 +++ 6 files changed, 738 insertions(+), 71 deletions(-) create mode 100644 drivers/usb/typec/ucsi/displayport.c -- 2.20.1
Re: [PATCH v5 2/2] usb: typec: ucsi: add firmware flashing support
On Thu, Feb 07, 2019 at 11:18:13AM -0800, Ajay Gupta wrote: > From: Ajay Gupta > > CCGx has two copies of the firmware in addition to the bootloader. > If the device is running FW1, FW2 can be updated with the new version. > Dual firmware mode allows the CCG device to stay in a PD contract and > support USB PD and Type-C functionality while a firmware update is in > progress. > > First we read the currently flashed firmware version of both > primary and secondary firmware and then compare it with > version of firmware file to determine if flashing is required. > > Command framework is added to support sending commands to CCGx > controller. We wait for response after sending the command and then > read the response from RAB_RESPONSE register. > > Below commands are supported, > - ENTER_FLASHING > - RESET > - PDPORT_ENABLE > - JUMP_TO_BOOT > - FLASH_ROW_RW > - VALIDATE_FW > > Command specific mutex lock is also added to sync between driver > and user threads. > > PD port number information is added which is required while sending > PD_PORT_ENABLE command > > Signed-off-by: Ajay Gupta Applied: https://github.com/krohei/linux/commit/6acc0b6b0b5041432016cd1c7adc8de20e80bf86 thanks, -- heikki
Re: [PATCH v5 1/2] usb: typec: ucsi: add get_fw_info function
On Thu, Feb 07, 2019 at 11:18:12AM -0800, Ajay Gupta wrote: > From: Ajay Gupta > > Function is to get the details of ccg firmware and device version. > It will be useful in debugging and also during firmware update. > > Signed-off-by: Ajay Gupta Applied: https://github.com/krohei/linux/commit/21dcedbca01c9e8532449a50861900e06c518ad2 thanks, -- heikki
Re: [PATCH v3 0/8] usb: typec: fusb302: Various fixes
On Fri, Mar 15, 2019 at 04:34:59PM +0200, Heikki Krogerus wrote: > On Mon, Mar 11, 2019 at 11:48:10AM +0100, Hans de Goede wrote: > > Hi All, > > > > Here is v3 of my fusb302 bug-fix series, the main fix in this series > > makes active adapters like Type-C to HDMI (often HDMI + USB-3-A) adapters > > work when they are to be powered by the Type-C port and thus present both > > an Ra and a Rd resistor on their Cc pins (patch 5). > > > > Changes in v3: > > > > [PATCH v3 4/8] usb: typec: fusb302: Check vconn is off when we start > > -Use WARN with a message describing the problem, instead of WARN_ON > > > > [PATCH v3 5/8] usb: typec: fusb302: Fix fusb302_handle_togdone_src Ra > > -Keep the delays for measuring Rd / Ra the same as before instead of > > multiplying them by a factor 100 > > > > [PATCH v3 7/8] usb: typec: fusb302: Improve suspend/resume handling > > -Change devm_request_irq to a regular request_irq free_irq, so that the work > > can be properly stopped on remove > > I tested these, and all of them look good to me: > > Acked-by: Heikki Krogerus I've also applied the entire series to my typec-next branch: https://github.com/krohei/linux/commits/typec-next thanks, -- heikki
Re: [PATCH] usb: typec: pi3usb30532: Keep orientation when setting mux to safe mode
On Fri, Mar 15, 2019 at 04:36:13PM +0200, Heikki Krogerus wrote:
> On Fri, Feb 22, 2019 at 08:22:39PM +0100, Hans de Goede wrote:
> > Keep the orientation value when setting the mux to safe mode, this
> > fixes the orientation getting reset when switching alt-modes.
> >
> > Signed-off-by: Hans de Goede
>
> Acked-by: Heikki Krogerus
Also applied to my typec-next branch:
https://github.com/krohei/linux/commit/4002f81b327b4537dead79ed6117dcb8065d2dca
> > ---
> > drivers/usb/typec/mux/pi3usb30532.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/typec/mux/pi3usb30532.c
> > b/drivers/usb/typec/mux/pi3usb30532.c
> > index 64eb5983e17a..9294e85fd34b 100644
> > --- a/drivers/usb/typec/mux/pi3usb30532.c
> > +++ b/drivers/usb/typec/mux/pi3usb30532.c
> > @@ -84,7 +84,8 @@ static int pi3usb30532_mux_set(struct typec_mux *mux, int
> > state)
> >
> > switch (state) {
> > case TYPEC_STATE_SAFE:
> > - new_conf = PI3USB30532_CONF_OPEN;
> > + new_conf = (new_conf & PI3USB30532_CONF_SWAP) |
> > + PI3USB30532_CONF_OPEN;
> > break;
> > case TYPEC_STATE_USB:
> > new_conf = (new_conf & PI3USB30532_CONF_SWAP) |
> > --
> > 2.20.1
thanks,
--
heikki
Re: [PATCH v2] usb: typec: tcpm: Try PD-2.0 if sink does not respond to 3.0 source-caps
On Sat, Mar 16, 2019 at 04:57:12PM +0100, Hans de Goede wrote:
> PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and
> simply ignore any src PDOs which the sink does not understand such as PPS
> but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message,
> causing contract negotiation to fail.
>
> This commit fixes such sinks not working by re-trying the contract
> negotiation with PD-2.0 source-caps messages if we don't have a contract
> after PD_N_HARD_RESET_COUNT hard-reset attempts.
>
> The problem fixed by this commit was noticed with a Type-C to VGA dongle.
>
> Signed-off-by: Hans de Goede
Reviewed-by: Heikki Krogerus
Also applied to my typec-next branch:
https://github.com/krohei/linux/commit/35e8c23e398ad03e7d9268a11a0408f8642da6d0
> ---
> The Type-C to VGA dongle on which this encountered looks like this one:
> https://www.aliexpress.com/item/Male-USB-3-1-Type-C-USB-C-to-Female-VGA-Adapter-Cable-10Gbps-for-New/32898274476.html
> ---
> Changes in v2:
> -Refactor the patch to avoid a check vs use race wrt negotiated_rev
> ---
> drivers/usb/typec/tcpm/tcpm.c | 27 ++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index f1c39a3c7534..d34e945e5d09 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -37,6 +37,7 @@
> S(SRC_ATTACHED),\
> S(SRC_STARTUP), \
> S(SRC_SEND_CAPABILITIES), \
> + S(SRC_SEND_CAPABILITIES_TIMEOUT), \
> S(SRC_NEGOTIATE_CAPABILITIES), \
> S(SRC_TRANSITION_SUPPLY), \
> S(SRC_READY), \
> @@ -2966,10 +2967,34 @@ static void run_state_machine(struct tcpm_port *port)
> /* port->hard_reset_count = 0; */
> port->caps_count = 0;
> port->pd_capable = true;
> - tcpm_set_state_cond(port, hard_reset_state(port),
> + tcpm_set_state_cond(port, SRC_SEND_CAPABILITIES_TIMEOUT,
> PD_T_SEND_SOURCE_CAP);
> }
> break;
> + case SRC_SEND_CAPABILITIES_TIMEOUT:
> + /*
> + * Error recovery for a PD_DATA_SOURCE_CAP reply timeout.
> + *
> + * PD 2.0 sinks are supposed to accept src-capabilities with a
> + * 3.0 header and simply ignore any src PDOs which the sink does
> + * not understand such as PPS but some 2.0 sinks instead ignore
> + * the entire PD_DATA_SOURCE_CAP message, causing contract
> + * negotiation to fail.
> + *
> + * After PD_N_HARD_RESET_COUNT hard-reset attempts, we try
> + * sending src-capabilities with a lower PD revision to
> + * make these broken sinks work.
> + */
> + if (port->hard_reset_count < PD_N_HARD_RESET_COUNT) {
> + tcpm_set_state(port, HARD_RESET_SEND, 0);
> + } else if (port->negotiated_rev > PD_REV20) {
> + port->negotiated_rev--;
> + port->hard_reset_count = 0;
> + tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0);
> + } else {
> + tcpm_set_state(port, hard_reset_state(port), 0);
> + }
> + break;
> case SRC_NEGOTIATE_CAPABILITIES:
> ret = tcpm_pd_check_request(port);
> if (ret < 0) {
> --
> 2.20.1
thanks,
--
heikki
[PATCH] usb: typec: wcove: Provide fwnode for the port
By registering a software fwnode for the port, we can supply
the connector capabilities to the tcpm using the common USB
connector device properties instead of relying on platform
data (struct tcpc_config).
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/tcpm/wcove.c | 29 -
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/typec/tcpm/wcove.c b/drivers/usb/typec/tcpm/wcove.c
index 423208e19383..53f00ad9aa92 100644
--- a/drivers/usb/typec/tcpm/wcove.c
+++ b/drivers/usb/typec/tcpm/wcove.c
@@ -587,17 +587,14 @@ static const u32 snk_pdo[] = {
PDO_VAR(5000, 12000, 3000),
};
-static struct tcpc_config wcove_typec_config = {
- .src_pdo = src_pdo,
- .nr_src_pdo = ARRAY_SIZE(src_pdo),
- .snk_pdo = snk_pdo,
- .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
-
- .operating_snk_mw = 15000,
-
- .type = TYPEC_PORT_DRP,
- .data = TYPEC_PORT_DRD,
- .default_role = TYPEC_SINK,
+static const struct property_entry wcove_props[] = {
+ PROPERTY_ENTRY_STRING("data-role", "dual"),
+ PROPERTY_ENTRY_STRING("power-role", "dual"),
+ PROPERTY_ENTRY_STRING("try-power-role", "sink"),
+ PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
+ PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
+ PROPERTY_ENTRY_U32("op-sink-microwatt", 15000),
+ { }
};
static int wcove_typec_probe(struct platform_device *pdev)
@@ -643,17 +640,22 @@ static int wcove_typec_probe(struct platform_device *pdev)
wcove->tcpc.set_roles = wcove_set_roles;
wcove->tcpc.pd_transmit = wcove_pd_transmit;
- wcove->tcpc.config = &wcove_typec_config;
+ wcove->tcpc.fwnode = fwnode_create_software_node(wcove_props, NULL);
+ if (IS_ERR(wcove->tcpc.fwnode))
+ return PTR_ERR(wcove->tcpc.fwnode);
wcove->tcpm = tcpm_register_port(wcove->dev, &wcove->tcpc);
- if (IS_ERR(wcove->tcpm))
+ if (IS_ERR(wcove->tcpm)) {
+ fwnode_remove_software_node(wcove->tcpc.fwnode);
return PTR_ERR(wcove->tcpm);
+ }
ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
wcove_typec_irq, IRQF_ONESHOT,
"wcove_typec", wcove);
if (ret) {
tcpm_unregister_port(wcove->tcpm);
+ fwnode_remove_software_node(wcove->tcpc.fwnode);
return ret;
}
@@ -673,6 +675,7 @@ static int wcove_typec_remove(struct platform_device *pdev)
regmap_write(wcove->regmap, USBC_IRQMASK2, val | USBC_IRQMASK2_ALL);
tcpm_unregister_port(wcove->tcpm);
+ fwnode_remove_software_node(wcove->tcpc.fwnode);
return 0;
}
--
2.20.1
Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO
On Mon, Mar 18, 2019 at 10:59:31AM +, Jun Li wrote: > > > > -Original Message- > > From: Heikki Krogerus > > Sent: 2019年3月13日 17:36 > > To: Rob Herring > > Cc: Jun Li ; gre...@linuxfoundation.org; > > hdego...@redhat.com; > > andy.shevche...@gmail.com; linux-usb@vger.kernel.org; > > devicet...@vger.kernel.org; dl-linux-imx > > Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec > > switch > > via GPIO > > > > On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote: > > > On Mon, Mar 11, 2019 at 10:40:09AM +, Jun Li wrote: > > > > Some typec super speed active channel switch can be controlled via a > > > > GPIO, this binding can be used to specify the switch node by a GPIO > > > > and the remote endpoint of its consumer. > > > > > > > > Signed-off-by: Li Jun > > > > --- > > > > .../devicetree/bindings/usb/typec-switch-gpio.txt | 30 > > > > ++ > > > > 1 file changed, 30 insertions(+) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt > > > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt > > > > new file mode 100644 > > > > index 000..4ef76cf > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt > > > > @@ -0,0 +1,30 @@ > > > > +Typec orientation switch via a GPIO > > > > +--- > > > > + > > > > +Required properties: > > > > +- compatible: should be set one of following: > > > > + - "nxp,ptn36043" for NXP Type-C SuperSpeed active switch. > > > > + > > > > +- gpios: the GPIO used to switch the super speed active channel, > > > > > > Perhaps switch-gpios in case there are other gpios needed. > > > > > > > + GPIO_ACTIVE_HIGH: GPIO state high for cc1; > > > > + GPIO_ACTIVE_LOW: GPIO state low for cc1. > > > > +- orientation-switch: must be present. > > > > > > Why is this needed? The compatible can't imply this? > > > > I think Jun Li is added that based on the comment I put to > > drivers/usb/typec/mux.c, > > so I'm to blame here. If we can handle this with the compatible like I > > guess we can, > > I'm happy. > > Hi Heikki > > Can I just remove the original bool property check? i.e, match OK if the > remote > parent node is in switch_list. No. If typec_switch_get() is called before the mux device is registered, we need to return -EPROBE_DEFER. That means we need to be able to identify the mux device node. I think we should just use the compatible like Rob suggested. The Type-C muxes should probable have their own bindings file where it's defined for these muxes. thanks, -- heikki
Re: [PATCH] usb: typec: altmodes/displayport: Fall back to multi-func pins
On Mon, Feb 25, 2019 at 01:56:37PM +0100, Hans de Goede wrote: > If our port-partner supports both DP-only operation (pin-assignment C) > and multi-func operation (pin-assignment D) and we only support > pin-assignment D and the port-partner prefers DP-only mode, then > before this commit we would and up masking out pin-assignment D from > the available pin-assignments and fail to pick a pin-assignment. > > Instead only mask out the multi-func pin-assignments if we support > dp-only pin-assignments, so that we correctly fall-back to a multi-func > pin-assignment in this case (by picking pin-assignment D). > > Signed-off-by: Hans de Goede Acked-by: Heikki Krogerus > --- > drivers/usb/typec/altmodes/displayport.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/altmodes/displayport.c > b/drivers/usb/typec/altmodes/displayport.c > index 3f06e94771a7..35161594e368 100644 > --- a/drivers/usb/typec/altmodes/displayport.c > +++ b/drivers/usb/typec/altmodes/displayport.c > @@ -104,7 +104,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 > con) > if (dp->data.status & DP_STATUS_PREFER_MULTI_FUNC && > pin_assign & DP_PIN_ASSIGN_MULTI_FUNC_MASK) > pin_assign &= DP_PIN_ASSIGN_MULTI_FUNC_MASK; > - else > + else if (pin_assign & DP_PIN_ASSIGN_DP_ONLY_MASK) > pin_assign &= DP_PIN_ASSIGN_DP_ONLY_MASK; > > if (!pin_assign) > -- > 2.20.1 thanks, -- heikki
Re: [PATCH] usb: typec: pi3usb30532: Keep orientation when setting mux to safe mode
On Fri, Feb 22, 2019 at 08:22:39PM +0100, Hans de Goede wrote:
> Keep the orientation value when setting the mux to safe mode, this
> fixes the orientation getting reset when switching alt-modes.
>
> Signed-off-by: Hans de Goede
Acked-by: Heikki Krogerus
> ---
> drivers/usb/typec/mux/pi3usb30532.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/mux/pi3usb30532.c
> b/drivers/usb/typec/mux/pi3usb30532.c
> index 64eb5983e17a..9294e85fd34b 100644
> --- a/drivers/usb/typec/mux/pi3usb30532.c
> +++ b/drivers/usb/typec/mux/pi3usb30532.c
> @@ -84,7 +84,8 @@ static int pi3usb30532_mux_set(struct typec_mux *mux, int
> state)
>
> switch (state) {
> case TYPEC_STATE_SAFE:
> - new_conf = PI3USB30532_CONF_OPEN;
> + new_conf = (new_conf & PI3USB30532_CONF_SWAP) |
> +PI3USB30532_CONF_OPEN;
> break;
> case TYPEC_STATE_USB:
> new_conf = (new_conf & PI3USB30532_CONF_SWAP) |
> --
> 2.20.1
thanks,
--
heikki
Re: [PATCH v3 0/8] usb: typec: fusb302: Various fixes
On Mon, Mar 11, 2019 at 11:48:10AM +0100, Hans de Goede wrote: > Hi All, > > Here is v3 of my fusb302 bug-fix series, the main fix in this series > makes active adapters like Type-C to HDMI (often HDMI + USB-3-A) adapters > work when they are to be powered by the Type-C port and thus present both > an Ra and a Rd resistor on their Cc pins (patch 5). > > Changes in v3: > > [PATCH v3 4/8] usb: typec: fusb302: Check vconn is off when we start > -Use WARN with a message describing the problem, instead of WARN_ON > > [PATCH v3 5/8] usb: typec: fusb302: Fix fusb302_handle_togdone_src Ra > -Keep the delays for measuring Rd / Ra the same as before instead of > multiplying them by a factor 100 > > [PATCH v3 7/8] usb: typec: fusb302: Improve suspend/resume handling > -Change devm_request_irq to a regular request_irq free_irq, so that the work > can be properly stopped on remove I tested these, and all of them look good to me: Acked-by: Heikki Krogerus thanks, -- heikki
Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO
On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote: > On Mon, Mar 11, 2019 at 10:40:09AM +, Jun Li wrote: > > Some typec super speed active channel switch can be controlled via > > a GPIO, this binding can be used to specify the switch node by > > a GPIO and the remote endpoint of its consumer. > > > > Signed-off-by: Li Jun > > --- > > .../devicetree/bindings/usb/typec-switch-gpio.txt | 30 > > ++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt > > new file mode 100644 > > index 000..4ef76cf > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt > > @@ -0,0 +1,30 @@ > > +Typec orientation switch via a GPIO > > +--- > > + > > +Required properties: > > +- compatible: should be set one of following: > > + - "nxp,ptn36043" for NXP Type-C SuperSpeed active switch. > > + > > +- gpios: the GPIO used to switch the super speed active channel, > > Perhaps switch-gpios in case there are other gpios needed. > > > + GPIO_ACTIVE_HIGH: GPIO state high for cc1; > > + GPIO_ACTIVE_LOW: GPIO state low for cc1. > > +- orientation-switch: must be present. > > Why is this needed? The compatible can't imply this? I think Jun Li is added that based on the comment I put to drivers/usb/typec/mux.c, so I'm to blame here. If we can handle this with the compatible like I guess we can, I'm happy. thanks, -- heikki
Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO
Hi,
On Tue, Mar 12, 2019 at 10:32:00AM +, Jun Li wrote:
> Hi Hans
> > -Original Message-
> > From: Hans de Goede
> > Sent: 2019年3月11日 19:03
> > To: Jun Li ; robh...@kernel.org;
> > heikki.kroge...@linux.intel.com
> > Cc: gre...@linuxfoundation.org; andy.shevche...@gmail.com;
> > linux-usb@vger.kernel.org; devicet...@vger.kernel.org; dl-linux-imx
> >
> > Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec
> > switch
> > via GPIO
> >
> > Hi,
> >
> > On 11-03-19 11:40, Jun Li wrote:
> > > Some typec super speed active channel switch can be controlled via a
> > > GPIO, this binding can be used to specify the switch node by a GPIO
> > > and the remote endpoint of its consumer.
> > >
> > > Signed-off-by: Li Jun
> > > ---
> > > .../devicetree/bindings/usb/typec-switch-gpio.txt | 30
> > ++
> > > 1 file changed, 30 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > new file mode 100644
> > > index 000..4ef76cf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > @@ -0,0 +1,30 @@
> > > +Typec orientation switch via a GPIO
> > > +---
> > > +
> > > +Required properties:
> > > +- compatible: should be set one of following:
> > > + - "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > > +
> > > +- gpios: the GPIO used to switch the super speed active channel,
> > > + GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > + GPIO_ACTIVE_LOW: GPIO state low for cc1.
> > > +- orientation-switch: must be present.
> >
> > Shouldn't this have usb-c in the propery name, e.g.:
> > usb-c-orientation-switch ?
>
> This is decided by drivers/usb/typec/mux.c:36
> /*
> * With OF graph the mux node must have a boolean device property named
> * "orientation-switch".
> */
Yes, but it's still OK to change it. It's not documented anywhere yet.
> > > +
> > > +Required sub-node:
> > > +- port: specify the remote endpoint of typec switch consumer.
> > > +
> > > +Example:
> > > +
> > > +ptn36043 {
> > > + compatible = "nxp,ptn36043";
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&pinctrl_ss_sel>;
> > > + gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > > + orientation-switch;
> > > +
> > > + port {
> > > + usb3_data_ss: endpoint {
> > > + remote-endpoint = <&typec_con_ss>;
> >
> >
> > Isn't this the wrong way around, shouldn't the "usb-c-connector"
> > compatible port be pointing to the orientation switch, rather then the
> > other way
> > around?
Hans, in OF graph both endpoints will have a remote-endpoint pointing
to each other..
> I am not sure I am getting your point, "usb-c-connector" is the user of typec
> switch,
> yes, it is pointing to the orientation switch provider(i.e, this example
> node).
>
> >Both will work in the end. but to me it feels more natural to group all the
> > info about the type-c connector together in the "usb-c-connector"
> > compatible port
> >
>
> ptn36043 {
> compatible = "nxp,ptn36043";
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_ss_sel>;
> gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> orientation-switch;
>
> port {
> usb3_data_ss: endpoint {
> remote-endpoint = <&typec_con_ss>;
> };
> };
> };
>
> usb_con: connector {
> compatible = "usb-c-connector";
> ...
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> port@1 {
> reg = <1>;
> typec_con_ss: endpoint {
> remote-endpoint = <&usb3_data_ss>;
> };
> };
> };
> };
So like that.
thanks,
--
heikki
Re: [PATCH v2 2/8] usb: typec: fusb302: Refactor / simplify tcpm_set_cc()
On Fri, Mar 08, 2019 at 11:58:32AM +0100, Hans de Goede wrote:
> Hi,
>
> On 08-03-19 08:33, Heikki Krogerus wrote:
> > On Thu, Mar 07, 2019 at 10:12:59AM -0800, Guenter Roeck wrote:
> > > On Thu, Mar 07, 2019 at 05:36:01PM +0100, Hans de Goede wrote:
> > > > After commit ea3b4d5523bc ("usb: typec: fusb302: Resolve fixed power
> > > > role
> > > > contract setup"), tcpm_set_cc always calls fusb302_set_toggling.
> > > >
> > > > Before this refactor tcpm_set_cc does the following:
> > > >
> > > > 1) fusb302_set_toggling(TOGGLING_MODE_OFF),
> > > > this sets both FUSB_REG_MASK_BC_LVL and FUSB_REG_MASK_COMP_CHNG.
> > > > 2) fusb302_set_cc_pull(...).
> > > > 3) "reset cc status".
> > > > 4) if pull-up fusb302_set_src_current(...).
> > > > 5) if pull-up or pull-down enable bc-lvl resp comp-chng irq.
> > > > 6) fusb302_set_toggling(new-toggling-mode), which again
> > > > sets both FUSB_REG_MASK_BC_LVL and FUSB_REG_MASK_COMP_CHNG disabling
> > > > the just enabled irq. fusb302_set_toggling is skipped when the new
> > > > toggling mode is TOGGLING_MODE_OFF because this is already done in
> > > > 1,
> > > > note in this case 5) is a no-op.
> > > >
> > > > When we are toggling the bits set by fusb302_set_cc_pull will be ignored
> > > > until we turn toggling off, so we can safely move the
> > > > fusb302_set_cc_pull
> > > > call to before setting TOGGLING_MODE_OFF.
> > > >
> > > > Either we are not toggling yet, or the src-current has already been set,
> > > > so we can also safely set the src-current earlier, allowing us to do the
> > > > fusb302_set_toggling(TOGGLING_MODE_OFF) call at the same time as we
> > > > set the other toggling modes. Also setting the src-current is a no-op
> > > > when not enabling pull-ups, so we can drop the if.
> > > >
> > > > And since the second fusb302_set_toggling undoes the effects of step 5,
> > > > we can skip step 5, the bc-lvl resp comp-chng irq wil be enabled by
> > > > fusb302_handle_togdone_snk resp. fusb302_handle_togdone_src when
> > > > toggling
> > > > is done.
> > > >
> > > > Together this allows us to simplify things to:
> > > >
> > > > 1) fusb302_set_cc_pull(...)
> > > > 2) "reset cc status"
> > > > 3) fusb302_set_src_current(...)
> > > > 4) fusb302_set_toggling(new-toggling-mode)
> > > >
> > > > This commit does this, leading to a nice cleanup.
> > > >
> > > > Signed-off-by: Hans de Goede
> > >
> > > I think I understand the logic, but I think it would be really beneficial
> > > if this (and the rest of the series) can be tested independently (ie
> > > by someone with hardware).
> > >
> > > Reviewed-by: Guenter Roeck
> >
> > I'll test this series with my GPD win board today.
>
> Note I've already tested this series on both a GPD win and a GPD pocket.
>
> Also Guenter's reviewed has shown that a small error has crept up
> in patch 5, patch 5 adds 2:
>
> usleep_range(5000, 1);
>
> lines which should be:
>
> usleep_range(50, 100);
>
> So if you test please make that change, or wait for v3 of the series.
OK.
thanks,
--
heikki
Re: [PATCH v2 2/8] usb: typec: fusb302: Refactor / simplify tcpm_set_cc()
On Thu, Mar 07, 2019 at 10:12:59AM -0800, Guenter Roeck wrote:
> On Thu, Mar 07, 2019 at 05:36:01PM +0100, Hans de Goede wrote:
> > After commit ea3b4d5523bc ("usb: typec: fusb302: Resolve fixed power role
> > contract setup"), tcpm_set_cc always calls fusb302_set_toggling.
> >
> > Before this refactor tcpm_set_cc does the following:
> >
> > 1) fusb302_set_toggling(TOGGLING_MODE_OFF),
> >this sets both FUSB_REG_MASK_BC_LVL and FUSB_REG_MASK_COMP_CHNG.
> > 2) fusb302_set_cc_pull(...).
> > 3) "reset cc status".
> > 4) if pull-up fusb302_set_src_current(...).
> > 5) if pull-up or pull-down enable bc-lvl resp comp-chng irq.
> > 6) fusb302_set_toggling(new-toggling-mode), which again
> >sets both FUSB_REG_MASK_BC_LVL and FUSB_REG_MASK_COMP_CHNG disabling
> >the just enabled irq. fusb302_set_toggling is skipped when the new
> >toggling mode is TOGGLING_MODE_OFF because this is already done in 1,
> >note in this case 5) is a no-op.
> >
> > When we are toggling the bits set by fusb302_set_cc_pull will be ignored
> > until we turn toggling off, so we can safely move the fusb302_set_cc_pull
> > call to before setting TOGGLING_MODE_OFF.
> >
> > Either we are not toggling yet, or the src-current has already been set,
> > so we can also safely set the src-current earlier, allowing us to do the
> > fusb302_set_toggling(TOGGLING_MODE_OFF) call at the same time as we
> > set the other toggling modes. Also setting the src-current is a no-op
> > when not enabling pull-ups, so we can drop the if.
> >
> > And since the second fusb302_set_toggling undoes the effects of step 5,
> > we can skip step 5, the bc-lvl resp comp-chng irq wil be enabled by
> > fusb302_handle_togdone_snk resp. fusb302_handle_togdone_src when toggling
> > is done.
> >
> > Together this allows us to simplify things to:
> >
> > 1) fusb302_set_cc_pull(...)
> > 2) "reset cc status"
> > 3) fusb302_set_src_current(...)
> > 4) fusb302_set_toggling(new-toggling-mode)
> >
> > This commit does this, leading to a nice cleanup.
> >
> > Signed-off-by: Hans de Goede
>
> I think I understand the logic, but I think it would be really beneficial
> if this (and the rest of the series) can be tested independently (ie
> by someone with hardware).
>
> Reviewed-by: Guenter Roeck
I'll test this series with my GPD win board today.
thanks,
--
heikki
Re: [PATCH 4/9] usb: gadget: udc: renesas_usb3: use extcon framework to receive connect/disconnect
On Wed, Mar 06, 2019 at 09:07:21AM +, Biju Das wrote: > RZ/G2E cat874 board is capable of detecting cable connect and disconnect > events. Add support for renesas_usb3 to receive connect and disconnect > notification using extcon framework. I think you will need the series from Yu Chen [1], but once we have that, you can do this with USB role class instead of relying on extcon, and that is what you need to do. There is a reason why we need to use a dedicated class with the USB role switching instead of relying on extcon. We actually originally planned on using extcon with the USB role switches, but the extcon maintainers refused some the USB mux drivers. The problem from extcon perspective is that the consumer/supplier roles seem to be inverted in some cases. USB role switch can simply be too many things - discrete mux, integrated mux (like in your case) or something like a dual role capable USB controller could simply represent one. On the other hand, using a dedicated API and class now feel like a much better idea in general compared to a multipurpose thing like extcon. [1] https://lkml.org/lkml/2019/3/2/42 thanks, -- heikki
Re: [PATCH 3/9] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller
Hi Biju,
On Wed, Mar 06, 2019 at 09:07:20AM +, Biju Das wrote:
> Driver for TI HD3SS3220 USB Type-C DRP port controller.
>
> The driver currently registers the port and supports data role swapping.
>
> Signed-off-by: Biju Das
> ---
> drivers/usb/typec/Kconfig | 10 ++
> drivers/usb/typec/Makefile| 1 +
> drivers/usb/typec/hd3ss3220.c | 284
> ++
> 3 files changed, 295 insertions(+)
> create mode 100644 drivers/usb/typec/hd3ss3220.c
>
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 30a847c..91696d2 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -49,6 +49,16 @@ source "drivers/usb/typec/tcpm/Kconfig"
>
> source "drivers/usb/typec/ucsi/Kconfig"
>
> +config TYPEC_HD3SS3220
> + tristate "TI HD3SS3220 Type-C DRP Port controller driver"
> + depends on I2C
> + help
> + Say Y or M here if your system has TI HD3SS3220 Type-C DRP Port
> + controller driver.
> +
> + If you choose to build this driver as a dynamically linked module, the
> + module will be called hd3ss3220.ko.
> +
> config TYPEC_TPS6598X
> tristate "TI TPS6598x USB Power Delivery controller driver"
> depends on I2C
> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> index 6696b72..7753a5c3 100644
> --- a/drivers/usb/typec/Makefile
> +++ b/drivers/usb/typec/Makefile
> @@ -4,5 +4,6 @@ typec-y := class.o mux.o bus.o
> obj-$(CONFIG_TYPEC) += altmodes/
> obj-$(CONFIG_TYPEC_TCPM) += tcpm/
> obj-$(CONFIG_TYPEC_UCSI) += ucsi/
> +obj-$(CONFIG_TYPEC_HD3SS3220)+= hd3ss3220.o
> obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
> obj-$(CONFIG_TYPEC) += mux/
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> new file mode 100644
> index 000..bd3e1ec
> --- /dev/null
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * TI HD3SS3220 Type-C DRP Port Controller Driver
> + *
> + * Copyright (C) 2019 Renesas Electronics Corp.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define HD3SS3220_REG_CN_STAT_CTRL 0x09
> +#define HD3SS3220_REG_GEN_CTRL 0x0A
> +#define HD3SS3220_REG_DEV_REV0xA0
> +
> +/* Register HD3SS3220_REG_CN_STAT_CTRL*/
> +#define HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK (BIT(7) |
> BIT(6))
> +#define HD3SS3220_REG_CN_STAT_CTRL_AS_DFPBIT(6)
> +#define HD3SS3220_REG_CN_STAT_CTRL_AS_UFPBIT(7)
> +#define HD3SS3220_REG_CN_STAT_CTRL_TO_ACCESSORY (BIT(7) |
> BIT(6))
> +#define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUSBIT(4)
> +
> +/* Register HD3SS3220_REG_GEN_CTRL*/
> +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK (BIT(2) | BIT(1))
> +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT 0x00
> +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK BIT(1)
> +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC (BIT(2) | BIT(1))
> +
> +struct hd3ss3220 {
> + struct device *dev;
> + struct regmap *regmap;
> + struct extcon_dev *extcon;
> + struct typec_port *port;
> + struct typec_capability typec_cap;
> +};
> +
> +static const unsigned int hd3ss3220_cable[] = {
> + EXTCON_USB,
> + EXTCON_USB_HOST,
> + EXTCON_NONE
> +};
You need to use the USB role class instead of extcon. Check
drivers/usb/roles/class/ and include/linux/usb/role.h
You may also want to check the updates Yu Chen is introducing to that
class:
https://lkml.org/lkml/2019/3/2/42
> +static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int
> src_pref)
> +{
> + unsigned int reg_val;
> + int ret;
> +
> + ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, ®_val);
> + if (ret < 0)
> + return ret;
> +
> + reg_val &= ~HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK;
> + reg_val |= src_pref;
> +
> + return regmap_write(hd3ss3220->regmap,
> + HD3SS3220_REG_GEN_CTRL, reg_val);
Please fix the alignment:
return regmap_write(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, reg_val);
> +}
> +
> +static int hd3ss3220_attached_state_detect(struct hd3ss3220 *hd3ss3220)
> +{
> + unsigned int reg_val;
> + int ret, attached_state;
> +
> + ret = regmap_read(hd3ss3220->regmap,
> + HD3SS3220_REG_CN_STAT_CTRL, ®_val);
ditto:
ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL,
®_val);
> + if (ret < 0)
> + return ret;
> +
> + reg_val &= HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK;
> + switch (reg_val) {
switch (reg_val & HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK)
> + case HD3SS3220_REG_CN_STAT_CTRL_AS_DFP:
> +
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi Hans, On Thu, Feb 28, 2019 at 05:54:21PM +0100, Hans de Goede wrote: > Hi Heikki, > > On 28-02-19 15:47, Heikki Krogerus wrote: > > Hi Hans, > > > > On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 28-02-19 10:15, Heikki Krogerus wrote: > > > > > > > I've been thinking about this... Do we actually need to link the > > > > correct drm_connector to the Type-C connector? Perhaps we can make > > > > this work by just linking the GFX device to the Type-C connector. > > > > > > What use is it to the kms driver if it gets an event there is a DP > > > hotplug with x lanes and orientation foo, if we are not telling it > > > on which DP port it is ? kms devices already have multiple DP ports > > > and more then one could be hooked-up to type-c connectors. > > > > I was looking at this series. You walk trough every DP port in the > > system when the DP alt mode driver broadcasts the event, but maybe > > that's different. Never mind. > > Right, my "simple / naive" solution simply tells the kms driver to > check all DP ports for connection state changes, similar to how > running "xrandr" under Xorg causes the kms driver to re-check the > connection status of all ports. Actually running xrandr under Xorg > after plugging in the cable, is how I did my initial DP over Type-C > testing on the GPD win. > > But once we start passing extra-info, I believe the kms driver needs > to know to which connector that info belongs. > > > > > > > Well, I don't think we can deny the GPU driver (in this case) the > > > > information that we have and that is relevant to it, just because it > > > > seems difficult to deliver that information to the right location. > > > > > > Right, but this does not require a tight-coupling. My original > > > proposal can do this if we pass a data struct with an identifier > > > for the DP port for which the event is to the notifier. I suggest using > > > a string for identifier, something like: ":00:02.0/DP-1" this > > > event struct could then also contain all the info we want to pass. > > > > I do agree that we should not tie the objects (device entries) > > representing these components in kernel together, but I assume that we > > agree now that the connection between the two - the USB Type-C > > connector and the DisplayPort - must be described somewhere, either in > > firmware or build-in? So I guess we are talking implementation details > > here, right? > > Right. > > > If that is the case... > > > > That string identifier you proposed would basically provide the > > details about the connection, so if we know those details, we might as > > well use "normal" ways to describe the connection and just extract > > them in runtime in the function that our DP alt mode driver calls. I > > think the connection has to be defined in i915 on CHT in any case. > > Interesting, I think the connection should be described in the fwnode > for the fusb302 device for the CHT/GPD win case. Specifically I think > this fits well as a property of the dp altmode. OK, you are correct. I was stupidly still thinking about the driver loading order, but the order does not matter. > > The DP alt mode driver should definitely not need to pass anything > > else to the notifier other than handle to itself (actually, handle > > straight to the port device would be better) as an identifier. The > > notifier function needs to be the one that determines the actual > > connection using that handle. Even if the target DP is described using > > a string like you propose, then that string has to come from > > somewhere, most likely from a device property. The notifier function > > can just as well extract it, we don't need to pass it separately. > > > > Here's my suggestion for function prototype: > > > > int drm_typec_dp_notification(struct device *typec_port_dev, > >struct typec_displayport_data *data); > > How about instead of the port_dev we pass in the altmode object and > we have a method to get the fwnode for the altmode? Then the > drm_typec_dp_notification() function can get info from that fwnode > to implement the connection finding you describe below: We can pass the altmode object, np, but let's not decide which fwnode we'll ultimately use. I'm still leaning towards the connector node. > > So that function finds the connection between typec_port_dev
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi Hans, On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote: > Hi, > > On 28-02-19 10:15, Heikki Krogerus wrote: > > On Wed, Feb 27, 2019 at 04:45:32PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 27-02-19 12:16, Jani Nikula wrote: > > > > On Wed, 27 Feb 2019, Heikki Krogerus > > > > wrote: > > > > > One thing that this series does not consider is the DP lane count > > > > > problem. The GPU drivers (i915 in this case) does not know is four, > > > > > two or one DP lanes in use. > > > > > > > > Also, orientation. > > > > > > The orientation should simply be correct with Type-C over DP. The mux / > > > orientation-switch used will take care of (physically) swapping the > > > lanes if the connector is inserted upside-down. > > > > > > > > I guess that is not a critical issue since there is a workaround (I > > > > > think) where the driver basically does trial and error, but ideally we > > > > > should be able to tell i915 also the pin assignment that was > > > > > negotiated with the partner device so it knows the DP lane count. > > > > > > > > Yeah, if the information is there, we'd like to know. > > > > > > So far machines actually using a setup where the kernel does the > > > USB-PD / Type-C negotiation rather then this being handled in firmware > > > in say the Alpine Ridge controller, are very rare. > > > > > > AFAIK in the Alpine Ridge controller case, there is no communication > > > with the i915 driver, the only thing the Alpine Ridge controller does > > > which the everything done in the kernel approach does not, is that > > > it actually has a pin connected to the HDP pin of the displayport in > > > question. But that just lets the i915 driver know about hotplug-events, > > > not how many lanes are used. > > > > > > Currently I'm aware of only 2 x86 models which actually need the > > > hotplug event propagation from the Type-C subsystem to the i915 driver. > > > Do we really want to come up with a much more complex solution to > > > optimize for this corner case, while the much more common case > > > (Alpine Ridge controller) does not provide this info to the i915 driver? > > > > The HPD signal is often handled with a GPIO on Intel Platforms. Either > > the PD controller or EC controller, after receiving the Attention > > message, triggers the GPIO. On some systems though that GPIO method > > could not be used, so instead a side channel communication is used: > > the GFX device (so i915 driver) receives a specific custom interrupt. > > > > Unfortunately it now appears that there may be products coming where > > using the GPIO is not going to be possible, and also side channel > > communication is going to be out of the question. I've started an > > internal discussion inside Intel about the possibility to extend the > > UCSI specification to be able to support that kind of products. > > > > Alpine Ridge uses TI's Power Delivery controllers. The platforms that > > have Alpine Ridge TBT controller(s) often expose the USB Type-C > > connectors on them to the OS via UCSI, however, it appears the Alpine > > Ridge actually allows direct access to the PD controllers from the OS. > > That would mean we can supply the same information to the GPU drivers > > that we will deliver on CHT also on every platform that uses Alpine > > Ridge. > > Ok. > > > > To give some idea of the complexity, first of all we need some > > > mechanism to let the kernel know which drm_connector is connected > > > to which Type-C port. For the 2 models I know if which need this, this > > > info is not available and we would need to hardcode it in the kernel > > > based on e.g. DMI matching. > > > > I've been thinking about this... Do we actually need to link the > > correct drm_connector to the Type-C connector? Perhaps we can make > > this work by just linking the GFX device to the Type-C connector. > > What use is it to the kms driver if it gets an event there is a DP > hotplug with x lanes and orientation foo, if we are not telling it > on which DP port it is ? kms devices already have multiple DP ports > and more then one could be hooked-up to type-c connectors. I was looking at this series. You walk trough every DP port in the system when the DP alt mode driver broadcasts the event, but maybe that's different. Never mind. > > > T
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
On Wed, Feb 27, 2019 at 04:45:32PM +0100, Hans de Goede wrote: > Hi, > > On 27-02-19 12:16, Jani Nikula wrote: > > On Wed, 27 Feb 2019, Heikki Krogerus > > wrote: > > > One thing that this series does not consider is the DP lane count > > > problem. The GPU drivers (i915 in this case) does not know is four, > > > two or one DP lanes in use. > > > > Also, orientation. > > The orientation should simply be correct with Type-C over DP. The mux / > orientation-switch used will take care of (physically) swapping the > lanes if the connector is inserted upside-down. > > > > I guess that is not a critical issue since there is a workaround (I > > > think) where the driver basically does trial and error, but ideally we > > > should be able to tell i915 also the pin assignment that was > > > negotiated with the partner device so it knows the DP lane count. > > > > Yeah, if the information is there, we'd like to know. > > So far machines actually using a setup where the kernel does the > USB-PD / Type-C negotiation rather then this being handled in firmware > in say the Alpine Ridge controller, are very rare. > > AFAIK in the Alpine Ridge controller case, there is no communication > with the i915 driver, the only thing the Alpine Ridge controller does > which the everything done in the kernel approach does not, is that > it actually has a pin connected to the HDP pin of the displayport in > question. But that just lets the i915 driver know about hotplug-events, > not how many lanes are used. > > Currently I'm aware of only 2 x86 models which actually need the > hotplug event propagation from the Type-C subsystem to the i915 driver. > Do we really want to come up with a much more complex solution to > optimize for this corner case, while the much more common case > (Alpine Ridge controller) does not provide this info to the i915 driver? The HPD signal is often handled with a GPIO on Intel Platforms. Either the PD controller or EC controller, after receiving the Attention message, triggers the GPIO. On some systems though that GPIO method could not be used, so instead a side channel communication is used: the GFX device (so i915 driver) receives a specific custom interrupt. Unfortunately it now appears that there may be products coming where using the GPIO is not going to be possible, and also side channel communication is going to be out of the question. I've started an internal discussion inside Intel about the possibility to extend the UCSI specification to be able to support that kind of products. Alpine Ridge uses TI's Power Delivery controllers. The platforms that have Alpine Ridge TBT controller(s) often expose the USB Type-C connectors on them to the OS via UCSI, however, it appears the Alpine Ridge actually allows direct access to the PD controllers from the OS. That would mean we can supply the same information to the GPU drivers that we will deliver on CHT also on every platform that uses Alpine Ridge. > To give some idea of the complexity, first of all we need some > mechanism to let the kernel know which drm_connector is connected > to which Type-C port. For the 2 models I know if which need this, this > info is not available and we would need to hardcode it in the kernel > based on e.g. DMI matching. I've been thinking about this... Do we actually need to link the correct drm_connector to the Type-C connector? Perhaps we can make this work by just linking the GFX device to the Type-C connector. > Then once we have this link, we need to start thinking about probe > ordering. Likely we need the typec framework to find the drm_connector, > since the typec-partner device is only created when there actually is > a DP capable "dongle" connected, making doing it the other way around > tricky. Then the typec-partner device needs to get a reference or some > such to make sure the drm_connector does not fgo away during the lifetime > of the typec-partner device. No! We must not link the partner device with anything other than the Type-C connector. We link the USB Type-C connector to the DisplayPort, and we link the USB Type-C connector to the partner. The Type-C connector is the proxy here. > I would really like to avoid this, so if we want to send more info to > the i915 driver, I suggest we create some event struct for this which > gets passed to the notifier, this could include a string to > describe the DP connector which the Type-C connector is connected to > when the mux is set to DP mode, e.g. "i915/DP-1" should be unique > or probably better, use the PCI device name, so: ":00:02.0/DP-1" > > Then we still have a loose coupling avoiding lifetime issues, while > the receiving drm driver can check whi
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
On Wed, Feb 27, 2019 at 01:16:27PM +0200, Jani Nikula wrote: > On Wed, 27 Feb 2019, Heikki Krogerus wrote: > > One thing that this series does not consider is the DP lane count > > problem. The GPU drivers (i915 in this case) does not know is four, > > two or one DP lanes in use. > > Also, orientation. > > > I guess that is not a critical issue since there is a workaround (I > > think) where the driver basically does trial and error, but ideally we > > should be able to tell i915 also the pin assignment that was > > negotiated with the partner device so it knows the DP lane count. > > Yeah, if the information is there, we'd like to know. With the > orientation, there's a worst case of sixth attempt of finding out > there's just one lane in a certain orientation. Couple that with link > rate selection (did it not work because too high link rate or because > the lanes are just not there?) we get pretty confused about what we > should try. The orientation is also considered in the alt mode API. We have a function for that typec_altmode_get_orientation(), but it of course requires that the caller has a handle to the alt mode object. So basically we would again need tight coupling between the DP connector and USB Type-C connector. Hans, I'm not so sure we should, or can, rule out the "tight coupling" option after all. thanks, -- heikki
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi Hans, On Mon, Feb 25, 2019 at 02:20:34PM +0100, Hans de Goede wrote: > Hi All, > > On some Cherry Trail devices, DisplayPort over Type-C is supported through > a USB-PD microcontroller (e.g. a fusb302) + a mux to switch the superspeed > datalines between USB-3 and DP (e.g. a pi3usb30532). The kernel in this > case does the PD/alt-mode negotiation itself, rather then everything being > handled in firmware. > > So the kernel itself picks an alt-mode, tells the Type-C "dongle" to switch > to DP mode and sets the mux accordingly. In this setup the HPD pin is not > connected, so the i915 driver needs to respond to a software event and scan > the DP port for changes manually. > > Thanks to Heikki's great work on the DisplayPort altmode support in the > typec subsys, we now correctly tell the dongle to switch to DP altmode > and we correctly set the mux and orientation switches to connect the > DP lines to the Type-C connector. > > This just leaves sending an out-of-band hotplug event from the Type-C > subsystem to the i915 driver and then we've fully working DP over Type-C > on these devices. > > This series implements this. The first patch adds a generic mechanism > for oob hotplug events to be send to the drm subsys, the second patch > adds support for this mechanism to the i915 driver and the third patch > makes the typec displayport_altmode driver send these events. > > The commit message of the first patch explains why I've chosen to things > the way these patches do them. One thing that this series does not consider is the DP lane count problem. The GPU drivers (i915 in this case) does not know is four, two or one DP lanes in use. I guess that is not a critical issue since there is a workaround (I think) where the driver basically does trial and error, but ideally we should be able to tell i915 also the pin assignment that was negotiated with the partner device so it knows the DP lane count. My original idea was that we pass that struct typec_displayport_data as payload in the event. From the Configuration VDO the GPU drivers can then see the negotiated DP pin assignment and determine the lane count. thanks, -- heikki
Re: [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events
On Mon, Feb 25, 2019 at 02:20:37PM +0100, Hans de Goede wrote: > Use the new drm_kms_call_oob_hotplug_notifier_chain() function to load > drm/kms drivers know about DisplayPort over Type-C hotplug events. > > Signed-off-by: Hans de Goede I'm OK with this. I'll wait for the v2 and see if I can test these. thanks, -- heikki
Re: [PATCH 2/2] usb: typec: add typec switch via GPIO control
On Mon, Feb 25, 2019 at 07:27:08AM +, Jun Li wrote: > This patch adds a simple typec switch driver which only needs > a GPIO to switch the super speed active channel according to > typec orientation. > > Signed-off-by: Li Jun > --- > drivers/usb/typec/mux/Kconfig | 6 +++ > drivers/usb/typec/mux/Makefile | 1 + > drivers/usb/typec/mux/gpio-switch.c | 105 > > 3 files changed, 112 insertions(+) > create mode 100644 drivers/usb/typec/mux/gpio-switch.c > > diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig > index 01ed0d5..bc7d3c7 100644 > --- a/drivers/usb/typec/mux/Kconfig > +++ b/drivers/usb/typec/mux/Kconfig > @@ -9,4 +9,10 @@ config TYPEC_MUX_PI3USB30532 > Say Y or M if your system has a Pericom PI3USB30532 Type-C cross > switch / mux chip found on some devices with a Type-C port. > > +config TYPEC_SWITCH_GPIO > + tristate "Simple Super Speed Active Switch via GPIO" depends on GPIOLIB? > + help > + Say Y or M if your system has a typec super speed channel > + switch via a simple GPIO control. > + thanks, -- heikki
Re: [PATCH] usb: typec: pi3usb30532: Keep orientation when setting mux to safe mode
On Mon, Feb 25, 2019 at 07:52:10PM +0100, Hans de Goede wrote: > Hi, > > On 25-02-19 16:50, Heikki Krogerus wrote: > > On Fri, Feb 22, 2019 at 08:22:39PM +0100, Hans de Goede wrote: > > > Keep the orientation value when setting the mux to safe mode, this > > > fixes the orientation getting reset when switching alt-modes. > > > > > > Signed-off-by: Hans de Goede > > > > Should this be also a fix? > > This only comes into play when switching alt-modes, so more > or less the same as with the displayport altmode fix: > > "There are no users if this yet, until we've agreement > on the DT bindings and code merged for adding alt-modes > to an usb-connector node, nothing will be using this code, > so I see little use in adding a Cc: stable or some such." Agree, thanks, -- heikki
Re: [PATCH] usb: typec: altmodes/displayport: Fall back to multi-func pins
On Mon, Feb 25, 2019 at 07:50:32PM +0100, Hans de Goede wrote: > Hi Heikki, > > On 25-02-19 16:49, Heikki Krogerus wrote: > > On Mon, Feb 25, 2019 at 01:56:37PM +0100, Hans de Goede wrote: > > > If our port-partner supports both DP-only operation (pin-assignment C) > > > and multi-func operation (pin-assignment D) and we only support > > > pin-assignment D and the port-partner prefers DP-only mode, then > > > before this commit we would and up masking out pin-assignment D from > > > the available pin-assignments and fail to pick a pin-assignment. > > > > > > Instead only mask out the multi-func pin-assignments if we support > > > dp-only pin-assignments, so that we correctly fall-back to a multi-func > > > pin-assignment in this case (by picking pin-assignment D). > > > > > > Signed-off-by: Hans de Goede > > > > Should this be handled as a fix? > > AFAIK they are no users if this yet, until we've agreement > on the DT bindings and code merged for adding alt-modes > to an usb-connector node, nothing will be using this code, > so I see little use in adding a Cc: stable or some such. True. thanks, -- heikki
Re: [PATCH] usb: typec: pi3usb30532: Keep orientation when setting mux to safe mode
On Fri, Feb 22, 2019 at 08:22:39PM +0100, Hans de Goede wrote:
> Keep the orientation value when setting the mux to safe mode, this
> fixes the orientation getting reset when switching alt-modes.
>
> Signed-off-by: Hans de Goede
Should this be also a fix?
> ---
> drivers/usb/typec/mux/pi3usb30532.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/mux/pi3usb30532.c
> b/drivers/usb/typec/mux/pi3usb30532.c
> index 64eb5983e17a..9294e85fd34b 100644
> --- a/drivers/usb/typec/mux/pi3usb30532.c
> +++ b/drivers/usb/typec/mux/pi3usb30532.c
> @@ -84,7 +84,8 @@ static int pi3usb30532_mux_set(struct typec_mux *mux, int
> state)
>
> switch (state) {
> case TYPEC_STATE_SAFE:
> - new_conf = PI3USB30532_CONF_OPEN;
> + new_conf = (new_conf & PI3USB30532_CONF_SWAP) |
> +PI3USB30532_CONF_OPEN;
> break;
> case TYPEC_STATE_USB:
> new_conf = (new_conf & PI3USB30532_CONF_SWAP) |
> --
> 2.20.1
thanks,
--
heikki
Re: [PATCH] usb: typec: altmodes/displayport: Fall back to multi-func pins
On Mon, Feb 25, 2019 at 01:56:37PM +0100, Hans de Goede wrote: > If our port-partner supports both DP-only operation (pin-assignment C) > and multi-func operation (pin-assignment D) and we only support > pin-assignment D and the port-partner prefers DP-only mode, then > before this commit we would and up masking out pin-assignment D from > the available pin-assignments and fail to pick a pin-assignment. > > Instead only mask out the multi-func pin-assignments if we support > dp-only pin-assignments, so that we correctly fall-back to a multi-func > pin-assignment in this case (by picking pin-assignment D). > > Signed-off-by: Hans de Goede Should this be handled as a fix? > --- > drivers/usb/typec/altmodes/displayport.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/altmodes/displayport.c > b/drivers/usb/typec/altmodes/displayport.c > index 3f06e94771a7..35161594e368 100644 > --- a/drivers/usb/typec/altmodes/displayport.c > +++ b/drivers/usb/typec/altmodes/displayport.c > @@ -104,7 +104,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 > con) > if (dp->data.status & DP_STATUS_PREFER_MULTI_FUNC && > pin_assign & DP_PIN_ASSIGN_MULTI_FUNC_MASK) > pin_assign &= DP_PIN_ASSIGN_MULTI_FUNC_MASK; > - else > + else if (pin_assign & DP_PIN_ASSIGN_DP_ONLY_MASK) > pin_assign &= DP_PIN_ASSIGN_DP_ONLY_MASK; > > if (!pin_assign) > -- > 2.20.1 thanks, -- heikki
Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters
On Wed, Feb 20, 2019 at 04:11:38PM +0100, Nikolaus Voss wrote:
> From: Nikolaus Voss
>
> Commit 1a2f474d328f handles block _reads_ separately with plain-I2C
> adapters, but the problem described with regmap-i2c not handling
> SMBus block transfers (i.e. read and writes) correctly also exists
> with writes.
>
> As workaround, this patch adds a block write function the same way
> 1a2f474d328f adds a block read function.
>
> Fixes: 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately
> with plain-I2C adapters")
> Fixes: 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery
> controllers")
> Signed-off-by: Nikolaus Voss
Acked-by: Heikki Krogerus
> ---
> v2: fix tps6598x_exec_cmd also
> v3: use fixed length for stack buffer
> ---
> drivers/usb/typec/tps6598x.c | 26 --
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> index c84c8c189e90..eb8046f87a54 100644
> --- a/drivers/usb/typec/tps6598x.c
> +++ b/drivers/usb/typec/tps6598x.c
> @@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void
> *val, size_t len)
> return 0;
> }
>
> +static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
> + void *val, size_t len)
> +{
> + u8 data[TPS_MAX_LEN + 1];
> +
> + if (!tps->i2c_protocol)
> + return regmap_raw_write(tps->regmap, reg, val, len);
> +
> + data[0] = len;
> + memcpy(&data[1], val, len);
> +
> + return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
> +}
> +
> static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
> {
> return tps6598x_block_read(tps, reg, val, sizeof(u16));
> @@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps,
> u8 reg, u64 *val)
>
> static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
> {
> - return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
> + return tps6598x_block_write(tps, reg, &val, sizeof(u16));
> }
>
> static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
> {
> - return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
> + return tps6598x_block_write(tps, reg, &val, sizeof(u32));
> }
>
> static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
> {
> - return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
> + return tps6598x_block_write(tps, reg, &val, sizeof(u64));
> }
>
> static inline int
> tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
> {
> - return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
> + return tps6598x_block_write(tps, reg, &val, sizeof(u32));
> }
>
> static int tps6598x_read_partner_identity(struct tps6598x *tps)
> @@ -229,8 +243,8 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const
> char *cmd,
> return -EBUSY;
>
> if (in_len) {
> - ret = regmap_raw_write(tps->regmap, TPS_REG_DATA1,
> -in_data, in_len);
> + ret = tps6598x_block_write(tps, TPS_REG_DATA1,
> +in_data, in_len);
> if (ret)
> return ret;
> }
> --
> 2.17.1
thanks,
--
heikki
Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters
On Wed, Feb 20, 2019 at 04:14:23PM +0200, Heikki Krogerus wrote:
> On Wed, Feb 20, 2019 at 02:38:47PM +0100, Nikolaus Voss wrote:
> > On Wed, 20 Feb 2019, Heikki Krogerus wrote:
> > > On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote:
> > > > Commit 1a2f474d328f handles block _reads_ separately with plain-I2C
> > > > adapters, but the problem described with regmap-i2c not handling
> > > > SMBus block transfers (i.e. read and writes) correctly also exists
> > > > with writes.
> > > >
> > > > As workaround, this patch adds a block write function the same way
> > > > 1a2f474d328f adds a block read function.
> > > >
> > > > Fixes: 1a2f474d328f ("usb: typec: tps6598x: handle block reads
> > > > separately with plain-I2C adapters")
> > > > Fixes: 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power
> > > > Delivery controllers")
> > > > Signed-off-by: Nikolaus Voss
> > >
> > > You are missing a "from" line with address that matches your SoB
> > > address.
> >
> > That's because I currently cannot send patch mails from my company account
> > as our MTA breaks diffs.
>
> I understand, but you can have a separate "From line" in your patch,
> i.e. you send the patch using one address, and have an extra "From
> line" (outside of the mail header) with another address.
>
> That other From line will be interpreted as the author address, and
> it should match your SoB address.
>
>
> Try something like this in a branch where this patch is the HEAD:
>
> % export MY_COMMIT=$(git show -s --pretty=%h HEAD)
> % git reset HEAD^
> % GIT_COMMITTER_IDENT='Nikolaus Voss
> ' \
> GIT_AUTHOR_IDENT='Nikolaus Voss
> ' \
> git commit -a -C $MY_COMMIT
Correction here:
% GIT_COMMITTER_IDENT='Nikolaus Voss
' \
GIT_AUTHOR_IDENT='Nikolaus Voss
' \
git commit -a -C $MY_COMMIT --reset-author
That "--reset-author" was missing. Sorry for that.
thanks,
--
heikki
Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters
On Wed, Feb 20, 2019 at 02:38:47PM +0100, Nikolaus Voss wrote:
> On Wed, 20 Feb 2019, Heikki Krogerus wrote:
> > On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote:
> > > Commit 1a2f474d328f handles block _reads_ separately with plain-I2C
> > > adapters, but the problem described with regmap-i2c not handling
> > > SMBus block transfers (i.e. read and writes) correctly also exists
> > > with writes.
> > >
> > > As workaround, this patch adds a block write function the same way
> > > 1a2f474d328f adds a block read function.
> > >
> > > Fixes: 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately
> > > with plain-I2C adapters")
> > > Fixes: 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power
> > > Delivery controllers")
> > > Signed-off-by: Nikolaus Voss
> >
> > You are missing a "from" line with address that matches your SoB
> > address.
>
> That's because I currently cannot send patch mails from my company account
> as our MTA breaks diffs.
I understand, but you can have a separate "From line" in your patch,
i.e. you send the patch using one address, and have an extra "From
line" (outside of the mail header) with another address.
That other From line will be interpreted as the author address, and
it should match your SoB address.
Try something like this in a branch where this patch is the HEAD:
% export MY_COMMIT=$(git show -s --pretty=%h HEAD)
% git reset HEAD^
% GIT_COMMITTER_IDENT='Nikolaus Voss
' \
GIT_AUTHOR_IDENT='Nikolaus Voss
' \
git commit -a -C $MY_COMMIT
Then:
% git format-patch HEAD^
% git send-email ...
thanks,
--
heikki
Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters
On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote:
> Commit 1a2f474d328f handles block _reads_ separately with plain-I2C
> adapters, but the problem described with regmap-i2c not handling
> SMBus block transfers (i.e. read and writes) correctly also exists
> with writes.
>
> As workaround, this patch adds a block write function the same way
> 1a2f474d328f adds a block read function.
>
> Fixes: 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately
> with plain-I2C adapters")
> Fixes: 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery
> controllers")
> Signed-off-by: Nikolaus Voss
You are missing a "from" line with address that matches your SoB
address.
thanks,
--
heikki
Re: [PATCH] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters
Hi,
On Mon, Sep 10, 2018 at 07:05:01AM +0200, Nikolaus Voss wrote:
> Commit 1a2f474d328f handles block _reads_ separately with plain-I2C
> adapters, but the problem described with regmap-i2c not handling
> SMBus block transfers (i.e. read and writes) correctly also exists
> with writes.
>
> As workaround, this patch adds a block write function the same way
> 1a2f474d328f adds a block read function.
>
> Fixes: 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately
> with plain-I2C adapters")
> Fixes: 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery
> controllers")
> Signed-off-by: Nikolaus Voss
> ---
> drivers/usb/typec/tps6598x.c | 22 ++
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> index c84c8c189e90..57a3e6c5c175 100644
> --- a/drivers/usb/typec/tps6598x.c
> +++ b/drivers/usb/typec/tps6598x.c
> @@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void
> *val, size_t len)
> return 0;
> }
>
> +static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
> + void *val, size_t len)
> +{
> + u8 data[len + 1];
> +
> + if (!tps->i2c_protocol)
> + return regmap_raw_write(tps->regmap, reg, val, len);
> +
> + data[0] = len;
> + memcpy(&data[1], val, len);
> +
> + return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
> +}
> +
> static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
> {
> return tps6598x_block_read(tps, reg, val, sizeof(u16));
> @@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps,
> u8 reg, u64 *val)
>
> static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
> {
> - return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
> + return tps6598x_block_write(tps, reg, &val, sizeof(u16));
> }
>
> static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
> {
> - return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
> + return tps6598x_block_write(tps, reg, &val, sizeof(u32));
> }
>
> static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
> {
> - return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
> + return tps6598x_block_write(tps, reg, &val, sizeof(u64));
> }
>
> static inline int
> tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
> {
> - return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
> + return tps6598x_block_write(tps, reg, &val, sizeof(u32));
> }
>
> static int tps6598x_read_partner_identity(struct tps6598x *tps)
You need to fix tps6598x_exec_cmd() as well.
Did you really send this last September? If you did, then the mail has
been stuck somewhere for a long time.
thanks,
--
heikki
Re: [PATCH][next] usb: typec: mux: fix an unsigned less than zero check
On Tue, Feb 19, 2019 at 03:19:18PM +, Colin King wrote:
> From: Colin Ian King
>
> The checks of a negative nval indicating an error an never be true
> as nval is currently a size_t which is of course unsigned and hence
> never less than zero. Fix this by making nval an int.
>
> Detected by CoverityScan, CID#1476863 ("Unsigned compared against 0)
> and CID#1476948 ("Loop bound")
>
> Fixes: 96a6d031ca99 ("usb: typec: mux: Find the muxes by also matching
> against the device node")
> Signed-off-by: Colin Ian King
> ---
> drivers/usb/typec/mux.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> index b94e2920eb38..64d2ed3fecb8 100644
> --- a/drivers/usb/typec/mux.c
> +++ b/drivers/usb/typec/mux.c
> @@ -126,10 +126,9 @@ static void *typec_mux_match(struct device_connection
> *con, int ep, void *data)
> {
> const struct typec_altmode_desc *desc = data;
> struct typec_mux *mux;
> - size_t nval;
> bool match;
> u16 *val;
> - int i;
> + int i, nval;
>
> if (!con->fwnode) {
> list_for_each_entry(mux, &mux_list, entry)
> --
> 2.20.1
This one has already a fix:
https://lkml.org/lkml/2019/2/19/56
thanks,
--
heikki
Re: [PATCH][next] usb: typec: mux: remove redundant check on variable match
On Tue, Feb 19, 2019 at 01:43:33PM +, Colin King wrote:
> From: Colin Ian King
>
> All the code paths that lead to the return statement are where
> match is always true, hence the check to see if it is true is
> redundant and can be removed.
>
> Detected by CoverityScan, CID#14769672 ("Logically dead code")
>
> Signed-off-by: Colin Ian King
Acked-by: Heikki Krogerus
> ---
> drivers/usb/typec/mux.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> index a5947d98824d..b94e2920eb38 100644
> --- a/drivers/usb/typec/mux.c
> +++ b/drivers/usb/typec/mux.c
> @@ -184,7 +184,7 @@ static void *typec_mux_match(struct device_connection
> *con, int ep, void *data)
> if (dev_fwnode(mux->dev) == con->fwnode)
> return mux;
>
> - return match ? ERR_PTR(-EPROBE_DEFER) : NULL;
> + return ERR_PTR(-EPROBE_DEFER);
> }
>
> /**
> --
> 2.20.1
thanks,
--
heikki
Re: [PATCH -next] usb: typec: mux: Fix unsigned comparison with less than zero
On Tue, Feb 19, 2019 at 02:57:50PM +0800, YueHaibing wrote:
> The return from the call to fwnode_property_read_u16_array is int,
> it can be a negative error code however this is being assigned to
> an size_t variable 'nval', hence the check is always false.
> Fix this by making 'nval' an int.
>
> Detected by Coccinelle ("Unsigned expression compared with
> zero: nval < 0")
>
> Fixes: 96a6d031ca99 ("usb: typec: mux: Find the muxes by also matching
> against the device node")
> Signed-off-by: YueHaibing
Acked-by: Heikki Krogerus
> ---
> drivers/usb/typec/mux.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> index a5947d9..54d7497 100644
> --- a/drivers/usb/typec/mux.c
> +++ b/drivers/usb/typec/mux.c
> @@ -126,7 +126,7 @@ static void *typec_mux_match(struct device_connection
> *con, int ep, void *data)
> {
> const struct typec_altmode_desc *desc = data;
> struct typec_mux *mux;
> - size_t nval;
> + int nval;
> bool match;
> u16 *val;
> int i;
thanks,
--
heikki
Re: [usb:usb-testing 63/64] drivers/usb/typec/mux.c:167 typec_mux_match() warn: unsigned 'nval' is never less than zero.
On Fri, Feb 15, 2019 at 07:51:56PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 15, 2019 at 4:40 PM Heikki Krogerus
> wrote:
> >
> > On Fri, Feb 15, 2019 at 11:19:47AM +0200, Andy Shevchenko wrote:
> > > On Fri, Feb 15, 2019 at 11:01 AM Heikki Krogerus
> > > wrote:
> > > >
> > > > On Fri, Feb 15, 2019 at 01:33:24PM +0800, kbuild test robot wrote:
> > > > > tree:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> > > > > usb-testing
> > > > > head: 09aa11cfda9d8186046bcd1adcd6498b688114f4
> > > > > commit: 96a6d031ca9930938bd66d0052fc7ed2b56e3583 [63/64] usb: typec:
> > > > > mux: Find the muxes by also matching against the device node
> > > > >
> > > > > smatch warnings:
> > > > > drivers/usb/typec/mux.c:167 typec_mux_match() warn: unsigned 'nval'
> > > > > is never less than zero.
> > > >
> > > > I'm assuming this means we should not use type 'size_t' in this case.
> > > >
> > > > I'll send a patch where I'll use 'int' instead if 'size_t'.
> > >
> > > Perhaps, in case of < 0 it should return it as error pointer to the
> > > caller?
> >
> > It already does that.
>
> Not in a line 159/160 AFAICS.
It get's tricky. We would then have to at least check that the error
value is not -ENODATA or -EINVAL, in those cases we should still
return NULL, but we can of course do that... Actually, now that I
think about it, we should return NULL also in case of -ENXIO. Even
that means we ignore the case.
I don't see a possible error path where we should return the error to
the caller when checking the number of elements.
> > > > > vim +/nval +167 drivers/usb/typec/mux.c
> > > > >
> > > > >124
> > > > >125static void *typec_mux_match(struct device_connection
> > > > > *con, int ep, void *data)
> > > > >126{
> > > > >127const struct typec_altmode_desc *desc = data;
> > > > >128struct typec_mux *mux;
> > > > >129size_t nval;
> > > > >130bool match;
> > > > >131u16 *val;
> > > > >132int i;
> > > > >133
> > > > >134if (!con->fwnode) {
> > > > >135list_for_each_entry(mux, &mux_list,
> > > > > entry)
> > > > >136if (!strcmp(con->endpoint[ep],
> > > > > dev_name(mux->dev)))
> > > > >137return mux;
> > > > >138return ERR_PTR(-EPROBE_DEFER);
> > > > >139}
> > > > >140
> > > > >141/*
> > > > >142 * Check has the identifier already been
> > > > > "consumed". If it
> > > > >143 * has, no need to do any extra connection
> > > > > identification.
> > > > >144 */
> > > > >145match = !con->id;
> > > > >146if (match)
> > > > >147goto find_mux;
> > > > >148
> > > > >149/* Accessory Mode muxes */
> > > > >150if (!desc) {
> > > > >151match =
> > > > > fwnode_property_present(con->fwnode, "accessory");
> > > > >152if (match)
> > > > >153goto find_mux;
> > > > >154return NULL;
> > > > >155}
> > > > >156
> > > > >157/* Alternate Mode muxes */
> > > > >158nval =
> > > > > fwnode_property_read_u16_array(con->fwnode, "svid", NULL, 0);
> > > > >159if (nval <= 0)
> > > > >160return NULL;
> > > > >161
> > > > >162val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
> > > > >163if (!val)
>
Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
Hi, On Sat, Feb 16, 2019 at 01:36:05AM +, Michael Hsu wrote: > > I think for clarity's sake I better fix the mode index issue now and send > > second > > version of these patches so we can concentrate on the bigger problem, the > > two > > connector DP alt modes. > > > > Agreed. Please update your patch for the mode index issue (being able to > reliably match partner mode index with the connector mode index returned by > UCSI). I'll finish some other task before coming back to this. We shouldn't be in any hurry with this. These are not going to v5.1 in any case. > > > Here's a method of avoiding the above dilemma: > > > - create partner sysfs alt mode nodes using UCSI GET_ALT_MODES (with > > > recipient == connector) > > > - also query UCSI GET_SUPPORTED_CAM > > > - create partner sysfs alt modes only if GET_SUPPORT_CAM returns 1 bit > > > for a particular connector alt mode index > > > > No! We must not hide stuff like that from the user. Even if the connector > > does > > not support a partner mode, we still show it to the user space. > > > > OK, but this means the "active" sysfs node for a partner mode (which is not > supported by GET_SUPPORTED_CAM) will always show "no". And writing "yes" > to it will silently fail... They will not always say "no", but you can not change the value without an alternate mode driver. > Did this intentional? Having some partner sysfs "active" nodes which do > nothing without any warning to user that they don't work? The key here is the driver. The user will know that partner alternate mode can't be operated unless it has been bind to a driver. The ABI for the alternate modes is defined for the typec bus, not the class. So in real world the user space applications will not start doing anything with an alternate mode, other than possible notifying the user of its existence, until the user space sees kernel event indicating that the alternate mode has been bind to a driver. Br, -- heikki
Re: [PATCH v3 0/9] device connection: Add support for device graphs
On Mon, Feb 18, 2019 at 09:07:27AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 15, 2019 at 02:47:15PM +0200, Heikki Krogerus wrote:
> > Hi Greg,
> >
> > On Thu, Feb 14, 2019 at 11:10:44AM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Feb 13, 2019 at 10:45:48AM +0300, Heikki Krogerus wrote:
> > > > Hi,
> > > >
> > > > This is the third version of my proposal to add device graph parsing
> > > > to the device connection API. There was only one problem reported in
> > > > v2 by Jun - kernel-doc entry was missing for the new fwnode member in
> > > > struct usb_role_switch_desc - and it's now fixed.
> > > >
> > > > The second version of the series:
> > > > https://lkml.org/lkml/2019/1/30/622
> > > >
> > > > The commit message from v1:
> > > >
> > > > This series adds support for OF and ACPI device graph parsing to the
> > > > device connection API.
> > > >
> > > > Handling the graph is straightforward, but because I'm adding that
> > > > fwnode member to struct device_connection, I had to make sure all the
> > > > existing users consider it.
> > > >
> > > > The plan is to only support matching with fwnode in the future, so no
> > > > more device name matching. The software fwnodes that we now have in
> > > > kernel should make that possible, once we add support for references
> > > > to them.
> > > >
> > > > The original RFC:
> > > > https://lkml.org/lkml/2018/10/24/619
> > >
> > > All now merged, thanks.
> >
> > It looks like you have not followed the order of the patches in this
> > series.
> >
> > You applied at least the patch 4/9 ("device connection: Add fwnode
> > member to struct device_connection") as the last patch to your
> > usb-next branch. The rest of the series, patches starting from 5/9,
> > depend on that patch.
>
> Ugh, did I just apply them out of order? I can't rewrite my tree now,
> sorry about that, odd that my sorting didn't work.
>
> I think all of the patches are now in the tree, so I didn't miss
> anything, is there anything I can do now? Want me to revert them and
> then add them back in the correct order?
The problem is with bisecting, so I'm not sure if reverting helps. I
don't think there's anything else that can be done about this now.
thanks,
--
heikki
Re: [usb:usb-testing 63/64] drivers/usb/typec/mux.c:167 typec_mux_match() warn: unsigned 'nval' is never less than zero.
On Fri, Feb 15, 2019 at 11:19:47AM +0200, Andy Shevchenko wrote:
> On Fri, Feb 15, 2019 at 11:01 AM Heikki Krogerus
> wrote:
> >
> > On Fri, Feb 15, 2019 at 01:33:24PM +0800, kbuild test robot wrote:
> > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> > > usb-testing
> > > head: 09aa11cfda9d8186046bcd1adcd6498b688114f4
> > > commit: 96a6d031ca9930938bd66d0052fc7ed2b56e3583 [63/64] usb: typec: mux:
> > > Find the muxes by also matching against the device node
> > >
> > > smatch warnings:
> > > drivers/usb/typec/mux.c:167 typec_mux_match() warn: unsigned 'nval' is
> > > never less than zero.
> >
> > I'm assuming this means we should not use type 'size_t' in this case.
> >
> > I'll send a patch where I'll use 'int' instead if 'size_t'.
>
> Perhaps, in case of < 0 it should return it as error pointer to the caller?
It already does that.
> > > vim +/nval +167 drivers/usb/typec/mux.c
> > >
> > >124
> > >125static void *typec_mux_match(struct device_connection *con,
> > > int ep, void *data)
> > >126{
> > >127const struct typec_altmode_desc *desc = data;
> > >128struct typec_mux *mux;
> > >129size_t nval;
> > >130bool match;
> > >131u16 *val;
> > >132int i;
> > >133
> > >134if (!con->fwnode) {
> > >135list_for_each_entry(mux, &mux_list, entry)
> > >136if (!strcmp(con->endpoint[ep],
> > > dev_name(mux->dev)))
> > >137return mux;
> > >138return ERR_PTR(-EPROBE_DEFER);
> > >139}
> > >140
> > >141/*
> > >142 * Check has the identifier already been
> > > "consumed". If it
> > >143 * has, no need to do any extra connection
> > > identification.
> > >144 */
> > >145match = !con->id;
> > >146if (match)
> > >147goto find_mux;
> > >148
> > >149/* Accessory Mode muxes */
> > >150if (!desc) {
> > >151match =
> > > fwnode_property_present(con->fwnode, "accessory");
> > >152if (match)
> > >153goto find_mux;
> > >154return NULL;
> > >155}
> > >156
> > >157/* Alternate Mode muxes */
> > >158nval = fwnode_property_read_u16_array(con->fwnode,
> > > "svid", NULL, 0);
> > >159if (nval <= 0)
> > >160return NULL;
> > >161
> > >162val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
> > >163if (!val)
> > >164return ERR_PTR(-ENOMEM);
> > >165
> > >166nval = fwnode_property_read_u16_array(con->fwnode,
> > > "svid", val, nval);
> > > > 167if (nval < 0) {
> > >168kfree(val);
> > >169return ERR_PTR(nval);
> > >170}
> > >171
> > >172for (i = 0; i < nval; i++) {
> > >173match = val[i] == desc->svid;
> > >174if (match) {
> > >175kfree(val);
> > >176goto find_mux;
> > >177}
> > >178}
> > >179kfree(val);
> > >180return NULL;
> > >181
> > >182find_mux:
> > >183list_for_each_entry(mux, &mux_list, entry)
> > >184if (dev_fwnode(mux->dev) == con->fwnode)
> > >185return mux;
> > >186
> > >187return match ? ERR_PTR(-EPROBE_DEFER) : NULL;
> > >188}
> > >189
thanks,
--
heikki
Re: [PATCH v3 0/9] device connection: Add support for device graphs
Hi Greg,
On Thu, Feb 14, 2019 at 11:10:44AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Feb 13, 2019 at 10:45:48AM +0300, Heikki Krogerus wrote:
> > Hi,
> >
> > This is the third version of my proposal to add device graph parsing
> > to the device connection API. There was only one problem reported in
> > v2 by Jun - kernel-doc entry was missing for the new fwnode member in
> > struct usb_role_switch_desc - and it's now fixed.
> >
> > The second version of the series:
> > https://lkml.org/lkml/2019/1/30/622
> >
> > The commit message from v1:
> >
> > This series adds support for OF and ACPI device graph parsing to the
> > device connection API.
> >
> > Handling the graph is straightforward, but because I'm adding that
> > fwnode member to struct device_connection, I had to make sure all the
> > existing users consider it.
> >
> > The plan is to only support matching with fwnode in the future, so no
> > more device name matching. The software fwnodes that we now have in
> > kernel should make that possible, once we add support for references
> > to them.
> >
> > The original RFC:
> > https://lkml.org/lkml/2018/10/24/619
>
> All now merged, thanks.
It looks like you have not followed the order of the patches in this
series.
You applied at least the patch 4/9 ("device connection: Add fwnode
member to struct device_connection") as the last patch to your
usb-next branch. The rest of the series, patches starting from 5/9,
depend on that patch.
thanks,
--
heikki
Re: [PATCH] usb: typec: tps6598x: Check mode of operation
On Thu, Feb 14, 2019 at 06:29:43PM +0300, Sergei Shtylyov wrote:
> On 02/14/2019 06:28 PM, Heikki Krogerus wrote:
>
> >>>>> +static int tps6598x_check_mode(struct tps6598x *tps)
> >>>>> +{
> >>>>> + char mode[5] = { };
> >>>>> + int ret;
> >>>>> +
> >>>>> + ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode);
> >>>>
> >>>>Casting pointers to 'void *' happens automagically, doesn't i?
> >>>
> >>> The third parameter in tps6598x_read32() is 'u32 *'.
> >>
> >>Then why cast to 'void *'?
> >
> > Because if we don't cast:
> >
> > drivers/usb/typec/tps6598x.c:408:43: error: passing argument 3 of
> > ‘tps6598x_read32’ from incompatible pointer type
>
>Why not cast to 'u32 *', I meant?
I always prefer (void *) in cases like this, and I believe I'm not the
only one. Though in this case, even if we later had to change the
tps6598x_read32() parameter to 'void *' from 'u32 *', it would not be
an issue to convert callers like this, but as a general rule, we
should not need to do that.
thanks,
--
heikki
Re: [usb:usb-testing 63/64] drivers/usb/typec/mux.c:167 typec_mux_match() warn: unsigned 'nval' is never less than zero.
On Fri, Feb 15, 2019 at 01:33:24PM +0800, kbuild test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> usb-testing
> head: 09aa11cfda9d8186046bcd1adcd6498b688114f4
> commit: 96a6d031ca9930938bd66d0052fc7ed2b56e3583 [63/64] usb: typec: mux:
> Find the muxes by also matching against the device node
>
> smatch warnings:
> drivers/usb/typec/mux.c:167 typec_mux_match() warn: unsigned 'nval' is never
> less than zero.
I'm assuming this means we should not use type 'size_t' in this case.
I'll send a patch where I'll use 'int' instead if 'size_t'.
> vim +/nval +167 drivers/usb/typec/mux.c
>
>124
>125static void *typec_mux_match(struct device_connection *con, int
> ep, void *data)
>126{
>127const struct typec_altmode_desc *desc = data;
>128struct typec_mux *mux;
>129size_t nval;
>130bool match;
>131u16 *val;
>132int i;
>133
>134if (!con->fwnode) {
>135list_for_each_entry(mux, &mux_list, entry)
>136if (!strcmp(con->endpoint[ep],
> dev_name(mux->dev)))
>137return mux;
>138return ERR_PTR(-EPROBE_DEFER);
>139}
>140
>141/*
>142 * Check has the identifier already been "consumed". If
> it
>143 * has, no need to do any extra connection
> identification.
>144 */
>145match = !con->id;
>146if (match)
>147goto find_mux;
>148
>149/* Accessory Mode muxes */
>150if (!desc) {
>151match = fwnode_property_present(con->fwnode,
> "accessory");
>152if (match)
>153goto find_mux;
>154return NULL;
>155}
>156
>157/* Alternate Mode muxes */
>158nval = fwnode_property_read_u16_array(con->fwnode,
> "svid", NULL, 0);
>159if (nval <= 0)
>160return NULL;
>161
>162val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
>163if (!val)
>164return ERR_PTR(-ENOMEM);
>165
>166nval = fwnode_property_read_u16_array(con->fwnode,
> "svid", val, nval);
> > 167if (nval < 0) {
>168kfree(val);
>169return ERR_PTR(nval);
>170}
>171
>172for (i = 0; i < nval; i++) {
>173match = val[i] == desc->svid;
>174if (match) {
>175kfree(val);
>176goto find_mux;
>177}
>178}
>179kfree(val);
>180return NULL;
>181
>182find_mux:
>183list_for_each_entry(mux, &mux_list, entry)
>184if (dev_fwnode(mux->dev) == con->fwnode)
>185return mux;
>186
>187return match ? ERR_PTR(-EPROBE_DEFER) : NULL;
>188}
>189
thanks,
--
heikki
Re: [PATCH] usb: typec: tps6598x: Check mode of operation
On Thu, Feb 14, 2019 at 06:22:12PM +0300, Sergei Shtylyov wrote:
> On 02/14/2019 03:12 PM, Heikki Krogerus wrote:
>
> >>> +static int tps6598x_check_mode(struct tps6598x *tps)
> >>> +{
> >>> + char mode[5] = { };
> >>> + int ret;
> >>> +
> >>> + ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode);
> >>
> >>Casting pointers to 'void *' happens automagically, doesn't i?
> >
> > The third parameter in tps6598x_read32() is 'u32 *'.
>
>Then why cast to 'void *'?
Because if we don't cast:
drivers/usb/typec/tps6598x.c:408:43: error: passing argument 3 of
‘tps6598x_read32’ from incompatible pointer type
--
heikki
Re: [PATCH] usb: typec: tps6598x: Check mode of operation
Hi Sergei,
On Thu, Feb 14, 2019 at 11:52:23AM +0300, Sergei Shtylyov wrote:
> > +static int tps6598x_check_mode(struct tps6598x *tps)
> > +{
> > + char mode[5] = { };
> > + int ret;
> > +
> > + ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode);
>
>Casting pointers to 'void *' happens automagically, doesn't i?
The third parameter in tps6598x_read32() is 'u32 *'.
thanks,
--
heikki
[PATCH] usb: typec: tps6598x: Check mode of operation
To prevent loading of the driver when the PD controller is
still in some operational mode that the driver does not
support, checking the mode in driver probe callback
function.
TI PD controllers may be in undefined mode of operation
for example when the application code (firmware) is
completely missing.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/tps6598x.c | 53 +---
1 file changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index 1c0033ad8738..9947c87d2a1e 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -14,6 +14,8 @@
#include
/* Register offsets */
+#define TPS_REG_VID0x00
+#define TPS_REG_MODE 0x03
#define TPS_REG_CMD1 0x08
#define TPS_REG_DATA1 0x09
#define TPS_REG_INT_EVENT1 0x14
@@ -66,6 +68,20 @@ struct tps6598x_rx_identity_reg {
#define TPS_TASK_TIMEOUT 1
#define TPS_TASK_REJECTED 3
+enum {
+ TPS_MODE_APP,
+ TPS_MODE_BOOT,
+ TPS_MODE_BIST,
+ TPS_MODE_DISC,
+};
+
+static const char *const modes[] = {
+ [TPS_MODE_APP] = "APP ",
+ [TPS_MODE_BOOT] = "BOOT",
+ [TPS_MODE_BIST] = "BIST",
+ [TPS_MODE_DISC] = "DISC",
+};
+
/* Unrecognized commands will be replaced with "!CMD" */
#define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321)
@@ -384,6 +400,32 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
return IRQ_HANDLED;
}
+static int tps6598x_check_mode(struct tps6598x *tps)
+{
+ char mode[5] = { };
+ int ret;
+
+ ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode);
+ if (ret)
+ return ret;
+
+ switch (match_string(modes, ARRAY_SIZE(modes), mode)) {
+ case TPS_MODE_APP:
+ return 0;
+ case TPS_MODE_BOOT:
+ dev_warn(tps->dev, "dead-battery condition\n");
+ return 0;
+ case TPS_MODE_BIST:
+ case TPS_MODE_DISC:
+ default:
+ dev_err(tps->dev, "controller in unsupported mode \"%s\"\n",
+ mode);
+ break;
+ }
+
+ return -ENODEV;
+}
+
static const struct regmap_config tps6598x_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
@@ -409,10 +451,8 @@ static int tps6598x_probe(struct i2c_client *client)
if (IS_ERR(tps->regmap))
return PTR_ERR(tps->regmap);
- ret = tps6598x_read32(tps, 0, &vid);
- if (ret < 0)
- return ret;
- if (!vid)
+ ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
+ if (ret < 0 || !vid)
return -ENODEV;
/*
@@ -425,6 +465,11 @@ static int tps6598x_probe(struct i2c_client *client)
if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
tps->i2c_protocol = true;
+ /* Make sure the controller has application firmware running */
+ ret = tps6598x_check_mode(tps);
+ if (ret)
+ return ret;
+
ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
if (ret < 0)
return ret;
--
2.20.1
[PATCH v3 7/9] usb: typec: Find the ports by also matching against the device node
When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.
Acked-by: Hans de Goede
Reviewed-by: Andy Shevchenko
Reviewed-by: Jun Li
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/class.c | 24 +---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 45abe2c7e9f3..2eb623841847 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include "bus.h"
@@ -204,15 +205,32 @@ static void typec_altmode_put_partner(struct altmode
*altmode)
put_device(&adev->dev);
}
-static int __typec_port_match(struct device *dev, const void *name)
+static int typec_port_fwnode_match(struct device *dev, const void *fwnode)
+{
+ return dev_fwnode(dev) == fwnode;
+}
+
+static int typec_port_name_match(struct device *dev, const void *name)
{
return !strcmp((const char *)name, dev_name(dev));
}
static void *typec_port_match(struct device_connection *con, int ep, void
*data)
{
- return class_find_device(typec_class, NULL, con->endpoint[ep],
-__typec_port_match);
+ struct device *dev;
+
+ /*
+* FIXME: Check does the fwnode supports the requested SVID. If it does
+* we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
+*/
+ if (con->fwnode)
+ return class_find_device(typec_class, NULL, con->fwnode,
+typec_port_fwnode_match);
+
+ dev = class_find_device(typec_class, NULL, con->endpoint[ep],
+ typec_port_name_match);
+
+ return dev ? dev : ERR_PTR(-EPROBE_DEFER);
}
struct typec_altmode *
--
2.20.1
[PATCH v3 9/9] device connection: Find device connections also from device graphs
If connections between devices are described in OF graph or
ACPI device graph, we can find them by using the
fwnode_graph_*() functions.
Acked-by: Hans de Goede
Reviewed-by: Andy Shevchenko
Reviewed-by: Jun Li
Tested-by: Jun Li
Signed-off-by: Heikki Krogerus
---
drivers/base/devcon.c | 38 +++---
1 file changed, 35 insertions(+), 3 deletions(-)
diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 858b8d2f6ef8..04db9ae235e4 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -7,10 +7,37 @@
*/
#include
+#include
static DEFINE_MUTEX(devcon_lock);
static LIST_HEAD(devcon_list);
+typedef void *(*devcon_match_fn_t)(struct device_connection *con, int ep,
+ void *data);
+
+static void *
+fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
+ void *data, devcon_match_fn_t match)
+{
+ struct device_connection con = { .id = con_id };
+ struct fwnode_handle *ep;
+ void *ret;
+
+ fwnode_graph_for_each_endpoint(fwnode, ep) {
+ con.fwnode = fwnode_graph_get_remote_port_parent(ep);
+ if (!fwnode_device_is_available(con.fwnode))
+ continue;
+
+ ret = match(&con, -1, data);
+ fwnode_handle_put(con.fwnode);
+ if (ret) {
+ fwnode_handle_put(ep);
+ return ret;
+ }
+ }
+ return NULL;
+}
+
/**
* device_connection_find_match - Find physical connection to a device
* @dev: Device with the connection
@@ -23,10 +50,9 @@ static LIST_HEAD(devcon_list);
* caller is expecting to be returned.
*/
void *device_connection_find_match(struct device *dev, const char *con_id,
- void *data,
- void *(*match)(struct device_connection *con,
- int ep, void *data))
+ void *data, devcon_match_fn_t match)
{
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
const char *devname = dev_name(dev);
struct device_connection *con;
void *ret = NULL;
@@ -35,6 +61,12 @@ void *device_connection_find_match(struct device *dev, const
char *con_id,
if (!match)
return NULL;
+ if (fwnode) {
+ ret = fwnode_graph_devcon_match(fwnode, con_id, data, match);
+ if (ret)
+ return ret;
+ }
+
mutex_lock(&devcon_lock);
list_for_each_entry(con, &devcon_list, list) {
--
2.20.1
[PATCH v3 0/9] device connection: Add support for device graphs
Hi, This is the third version of my proposal to add device graph parsing to the device connection API. There was only one problem reported in v2 by Jun - kernel-doc entry was missing for the new fwnode member in struct usb_role_switch_desc - and it's now fixed. The second version of the series: https://lkml.org/lkml/2019/1/30/622 The commit message from v1: This series adds support for OF and ACPI device graph parsing to the device connection API. Handling the graph is straightforward, but because I'm adding that fwnode member to struct device_connection, I had to make sure all the existing users consider it. The plan is to only support matching with fwnode in the future, so no more device name matching. The software fwnodes that we now have in kernel should make that possible, once we add support for references to them. The original RFC: https://lkml.org/lkml/2018/10/24/619 thanks, Heikki Krogerus (9): platform/x86: intel_cht_int33fe: Prepare for better mux naming scheme usb: typec: Rationalize the API for the muxes platform/x86: intel_cht_int33fe: Remove old style mux connections device connection: Add fwnode member to struct device_connection usb: typec: mux: Find the muxes by also matching against the device node usb: roles: Find the muxes by also matching against the device node usb: typec: Find the ports by also matching against the device node device connection: Prepare support for firmware described connections device connection: Find device connections also from device graphs drivers/base/devcon.c| 62 ++- drivers/platform/x86/intel_cht_int33fe.c | 15 ++-- drivers/usb/roles/class.c| 21 +- drivers/usb/typec/class.c| 31 ++-- drivers/usb/typec/mux.c | 96 include/linux/device.h | 6 ++ include/linux/usb/role.h | 2 + include/linux/usb/typec_mux.h| 3 +- 8 files changed, 196 insertions(+), 40 deletions(-) -- 2.20.1
[PATCH v3 6/9] usb: roles: Find the muxes by also matching against the device node
When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.
Acked-by: Hans de Goede
Reviewed-by: Andy Shevchenko
Reviewed-by: Jun Li
Tested-by: Jun Li
Signed-off-by: Heikki Krogerus
---
drivers/usb/roles/class.c | 21 ++---
include/linux/usb/role.h | 2 ++
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index 99116af07f1d..f45d8df5cfb8 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -8,6 +8,7 @@
*/
#include
+#include
#include
#include
#include
@@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
usb_role_switch *sw)
}
EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
-static int __switch_match(struct device *dev, const void *name)
+static int switch_fwnode_match(struct device *dev, const void *fwnode)
+{
+ return dev_fwnode(dev) == fwnode;
+}
+
+static int switch_name_match(struct device *dev, const void *name)
{
return !strcmp((const char *)name, dev_name(dev));
}
@@ -94,8 +100,16 @@ static void *usb_role_switch_match(struct device_connection
*con, int ep,
{
struct device *dev;
- dev = class_find_device(role_class, NULL, con->endpoint[ep],
- __switch_match);
+ if (con->fwnode) {
+ if (!fwnode_property_present(con->fwnode, con->id))
+ return NULL;
+
+ dev = class_find_device(role_class, NULL, con->fwnode,
+ switch_fwnode_match);
+ } else {
+ dev = class_find_device(role_class, NULL, con->endpoint[ep],
+ switch_name_match);
+ }
return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
}
@@ -266,6 +280,7 @@ usb_role_switch_register(struct device *parent,
sw->get = desc->get;
sw->dev.parent = parent;
+ sw->dev.fwnode = desc->fwnode;
sw->dev.class = role_class;
sw->dev.type = &usb_role_dev_type;
dev_set_name(&sw->dev, "%s-role-switch", dev_name(parent));
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index edc51be4a77c..c05ffa6abda9 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -18,6 +18,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct device
*dev);
/**
* struct usb_role_switch_desc - USB Role Switch Descriptor
+ * @fwnode: The device node to be associated with the role switch
* @usb2_port: Optional reference to the host controller port device (USB2)
* @usb3_port: Optional reference to the host controller port device (USB3)
* @udc: Optional reference to the peripheral controller device
@@ -32,6 +33,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct device
*dev);
* usb_role_switch_register() before registering the switch.
*/
struct usb_role_switch_desc {
+ struct fwnode_handle *fwnode;
struct device *usb2_port;
struct device *usb3_port;
struct device *udc;
--
2.20.1
[PATCH v3 2/9] usb: typec: Rationalize the API for the muxes
Since with accessory modes there is no need for additional
identification when requesting a handle to the mux, we can
replace the second parameter that is passed to the
typec_mux_get() function with a pointer to alternate mode
description structure, and simply passing NULL with
accessory modes.
This change means the naming of the mux device connections
can be updated. Alternate and Accessory Modes will both be
handled with muxes named "mode-switch", and the orientation
switches will be named "orientation-switch".
Future identification of the alternate modes will be later
done using device property "svid" of the mux.
Reviewed-by: Hans de Goede
Reviewed-by: Andy Shevchenko
Reviewed-by: Jun Li
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/class.c | 7 ++-
drivers/usb/typec/mux.c | 10 ++
include/linux/usb/typec_mux.h | 3 ++-
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 41c0d790a50f..45abe2c7e9f3 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1496,11 +1496,8 @@ typec_port_register_altmode(struct typec_port *port,
{
struct typec_altmode *adev;
struct typec_mux *mux;
- char id[10];
- sprintf(id, "id%04xm%02x", desc->svid, desc->mode);
-
- mux = typec_mux_get(&port->dev, id);
+ mux = typec_mux_get(&port->dev, desc);
if (IS_ERR(mux))
return ERR_CAST(mux);
@@ -1593,7 +1590,7 @@ struct typec_port *typec_register_port(struct device
*parent,
return ERR_CAST(port->sw);
}
- port->mux = typec_mux_get(&port->dev, "typec-mux");
+ port->mux = typec_mux_get(&port->dev, NULL);
if (IS_ERR(port->mux)) {
put_device(&port->dev);
return ERR_CAST(port->mux);
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index d990aa510fab..8975f58e1d60 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -48,7 +48,7 @@ struct typec_switch *typec_switch_get(struct device *dev)
struct typec_switch *sw;
mutex_lock(&switch_lock);
- sw = device_connection_find_match(dev, "typec-switch", NULL,
+ sw = device_connection_find_match(dev, "orientation-switch", NULL,
typec_switch_match);
if (!IS_ERR_OR_NULL(sw)) {
WARN_ON(!try_module_get(sw->dev->driver->owner));
@@ -128,19 +128,21 @@ static void *typec_mux_match(struct device_connection
*con, int ep, void *data)
/**
* typec_mux_get - Find USB Type-C Multiplexer
* @dev: The caller device
- * @name: Mux identifier
+ * @desc: Alt Mode description
*
* Finds a mux linked to the caller. This function is primarily meant for the
* Type-C drivers. Returns a reference to the mux on success, NULL if no
* matching connection was found, or ERR_PTR(-EPROBE_DEFER) when a connection
* was found but the mux has not been enumerated yet.
*/
-struct typec_mux *typec_mux_get(struct device *dev, const char *name)
+struct typec_mux *typec_mux_get(struct device *dev,
+ const struct typec_altmode_desc *desc)
{
struct typec_mux *mux;
mutex_lock(&mux_lock);
- mux = device_connection_find_match(dev, name, NULL, typec_mux_match);
+ mux = device_connection_find_match(dev, "mode-switch", (void *)desc,
+ typec_mux_match);
if (!IS_ERR_OR_NULL(mux)) {
WARN_ON(!try_module_get(mux->dev->driver->owner));
get_device(mux->dev);
diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
index 79293f630ee1..43f40685e53c 100644
--- a/include/linux/usb/typec_mux.h
+++ b/include/linux/usb/typec_mux.h
@@ -47,7 +47,8 @@ void typec_switch_put(struct typec_switch *sw);
int typec_switch_register(struct typec_switch *sw);
void typec_switch_unregister(struct typec_switch *sw);
-struct typec_mux *typec_mux_get(struct device *dev, const char *name);
+struct typec_mux *
+typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc);
void typec_mux_put(struct typec_mux *mux);
int typec_mux_register(struct typec_mux *mux);
void typec_mux_unregister(struct typec_mux *mux);
--
2.20.1
[PATCH v3 1/9] platform/x86: intel_cht_int33fe: Prepare for better mux naming scheme
Adding new connections with for the muxes with new
identifiers. The old connection are left in for now.
Reviewed-by: Hans de Goede
Reviewed-by: Andy Shevchenko
Reviewed-by: Jun Li
Signed-off-by: Heikki Krogerus
---
drivers/platform/x86/intel_cht_int33fe.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/intel_cht_int33fe.c
b/drivers/platform/x86/intel_cht_int33fe.c
index 02bc74608cf3..295fe19ad4c2 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -32,7 +32,7 @@ struct cht_int33fe_data {
struct i2c_client *fusb302;
struct i2c_client *pi3usb30532;
/* Contain a list-head must be per device */
- struct device_connection connections[5];
+ struct device_connection connections[7];
};
/*
@@ -184,6 +184,12 @@ static int cht_int33fe_probe(struct platform_device *pdev)
data->connections[3].endpoint[0] = "i2c-fusb302";
data->connections[3].endpoint[1] = "intel_xhci_usb_sw-role-switch";
data->connections[3].id = "usb-role-switch";
+ data->connections[4].endpoint[0] = "port0";
+ data->connections[4].endpoint[1] = "i2c-pi3usb30532";
+ data->connections[4].id = "orientation-switch";
+ data->connections[5].endpoint[0] = "port0";
+ data->connections[5].endpoint[1] = "i2c-pi3usb30532";
+ data->connections[5].id = "mode-switch";
device_connections_add(data->connections);
--
2.20.1
[PATCH v3 8/9] device connection: Prepare support for firmware described connections
When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device. The endpoint member for the device names will not be
used at all in that case.
Acked-by: Hans de Goede
Reviewed-by: Andy Shevchenko
Reviewed-by: Jun Li
Tested-by: Jun Li
Signed-off-by: Heikki Krogerus
---
drivers/base/devcon.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index d427e806cd73..858b8d2f6ef8 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -75,12 +75,36 @@ static struct bus_type *generic_match_buses[] = {
NULL,
};
+static int device_fwnode_match(struct device *dev, void *fwnode)
+{
+ return dev_fwnode(dev) == fwnode;
+}
+
+static void *device_connection_fwnode_match(struct device_connection *con)
+{
+ struct bus_type *bus;
+ struct device *dev;
+
+ for (bus = generic_match_buses[0]; bus; bus++) {
+ dev = bus_find_device(bus, NULL, (void *)con->fwnode,
+ device_fwnode_match);
+ if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id)))
+ return dev;
+
+ put_device(dev);
+ }
+ return NULL;
+}
+
/* This tries to find the device from the most common bus types by name. */
static void *generic_match(struct device_connection *con, int ep, void *data)
{
struct bus_type *bus;
struct device *dev;
+ if (con->fwnode)
+ return device_connection_fwnode_match(con);
+
for (bus = generic_match_buses[0]; bus; bus++) {
dev = bus_find_device_by_name(bus, NULL, con->endpoint[ep]);
if (dev)
--
2.20.1
[PATCH v3 3/9] platform/x86: intel_cht_int33fe: Remove old style mux connections
The new mux connection naming scheme is now in use, so
dropping the connections still using the old names. From now
on the same connection description named "mode-switch" is
used with both the port and the alternate modes, so on CHT
the DP alt mode will use the same connection as the port to
get a handle to the mux device.
Reviewed-by: Hans de Goede
Reviewed-by: Andy Shevchenko
Reviewed-by: Jun Li
Signed-off-by: Heikki Krogerus
---
drivers/platform/x86/intel_cht_int33fe.c | 21 ++---
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/drivers/platform/x86/intel_cht_int33fe.c
b/drivers/platform/x86/intel_cht_int33fe.c
index 295fe19ad4c2..6fa3cced6f8e 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -32,7 +32,7 @@ struct cht_int33fe_data {
struct i2c_client *fusb302;
struct i2c_client *pi3usb30532;
/* Contain a list-head must be per device */
- struct device_connection connections[7];
+ struct device_connection connections[4];
};
/*
@@ -174,22 +174,13 @@ static int cht_int33fe_probe(struct platform_device *pdev)
data->connections[0].endpoint[0] = "port0";
data->connections[0].endpoint[1] = "i2c-pi3usb30532";
- data->connections[0].id = "typec-switch";
+ data->connections[0].id = "orientation-switch";
data->connections[1].endpoint[0] = "port0";
data->connections[1].endpoint[1] = "i2c-pi3usb30532";
- data->connections[1].id = "typec-mux";
- data->connections[2].endpoint[0] = "port0";
- data->connections[2].endpoint[1] = "i2c-pi3usb30532";
- data->connections[2].id = "idff01m01";
- data->connections[3].endpoint[0] = "i2c-fusb302";
- data->connections[3].endpoint[1] = "intel_xhci_usb_sw-role-switch";
- data->connections[3].id = "usb-role-switch";
- data->connections[4].endpoint[0] = "port0";
- data->connections[4].endpoint[1] = "i2c-pi3usb30532";
- data->connections[4].id = "orientation-switch";
- data->connections[5].endpoint[0] = "port0";
- data->connections[5].endpoint[1] = "i2c-pi3usb30532";
- data->connections[5].id = "mode-switch";
+ data->connections[1].id = "mode-switch";
+ data->connections[2].endpoint[0] = "i2c-fusb302";
+ data->connections[2].endpoint[1] = "intel_xhci_usb_sw-role-switch";
+ data->connections[2].id = "usb-role-switch";
device_connections_add(data->connections);
--
2.20.1
[PATCH v3 4/9] device connection: Add fwnode member to struct device_connection
This will prepare the device connection API for connections
described in firmware.
Acked-by: Hans de Goede
Reviewed-by: Andy Shevchenko
Reviewed-by: Jun Li
Signed-off-by: Heikki Krogerus
---
include/linux/device.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/include/linux/device.h b/include/linux/device.h
index d4e74fa42f9c..333db8c06152 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -758,11 +758,17 @@ struct device_dma_parameters {
/**
* struct device_connection - Device Connection Descriptor
+ * @fwnode: The device node of the connected device
* @endpoint: The names of the two devices connected together
* @id: Unique identifier for the connection
* @list: List head, private, for internal use only
+ *
+ * NOTE: @fwnode is not used together with @endpoint. @fwnode is used when
+ * platform firmware defines the connection. When the connection is registered
+ * with device_connection_add() @endpoint is used instead.
*/
struct device_connection {
+ struct fwnode_handle*fwnode;
const char *endpoint[2];
const char *id;
struct list_headlist;
--
2.20.1
[PATCH v3 5/9] usb: typec: mux: Find the muxes by also matching against the device node
When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.
Acked-by: Hans de Goede
Reviewed-by: Andy Shevchenko
Reviewed-by: Jun Li
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/mux.c | 86 +++--
1 file changed, 74 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index 8975f58e1d60..a5947d98824d 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -11,6 +11,8 @@
#include
#include
#include
+#include
+#include
#include
static DEFINE_MUTEX(switch_lock);
@@ -23,15 +25,25 @@ static void *typec_switch_match(struct device_connection
*con, int ep,
{
struct typec_switch *sw;
- list_for_each_entry(sw, &switch_list, entry)
- if (!strcmp(con->endpoint[ep], dev_name(sw->dev)))
- return sw;
+ if (!con->fwnode) {
+ list_for_each_entry(sw, &switch_list, entry)
+ if (!strcmp(con->endpoint[ep], dev_name(sw->dev)))
+ return sw;
+ return ERR_PTR(-EPROBE_DEFER);
+ }
/*
-* We only get called if a connection was found, tell the caller to
-* wait for the switch to show up.
+* With OF graph the mux node must have a boolean device property named
+* "orientation-switch".
*/
- return ERR_PTR(-EPROBE_DEFER);
+ if (con->id && !fwnode_property_present(con->fwnode, con->id))
+ return NULL;
+
+ list_for_each_entry(sw, &switch_list, entry)
+ if (dev_fwnode(sw->dev) == con->fwnode)
+ return sw;
+
+ return con->id ? ERR_PTR(-EPROBE_DEFER) : NULL;
}
/**
@@ -112,17 +124,67 @@ EXPORT_SYMBOL_GPL(typec_switch_unregister);
static void *typec_mux_match(struct device_connection *con, int ep, void *data)
{
+ const struct typec_altmode_desc *desc = data;
struct typec_mux *mux;
+ size_t nval;
+ bool match;
+ u16 *val;
+ int i;
- list_for_each_entry(mux, &mux_list, entry)
- if (!strcmp(con->endpoint[ep], dev_name(mux->dev)))
- return mux;
+ if (!con->fwnode) {
+ list_for_each_entry(mux, &mux_list, entry)
+ if (!strcmp(con->endpoint[ep], dev_name(mux->dev)))
+ return mux;
+ return ERR_PTR(-EPROBE_DEFER);
+ }
/*
-* We only get called if a connection was found, tell the caller to
-* wait for the switch to show up.
+* Check has the identifier already been "consumed". If it
+* has, no need to do any extra connection identification.
*/
- return ERR_PTR(-EPROBE_DEFER);
+ match = !con->id;
+ if (match)
+ goto find_mux;
+
+ /* Accessory Mode muxes */
+ if (!desc) {
+ match = fwnode_property_present(con->fwnode, "accessory");
+ if (match)
+ goto find_mux;
+ return NULL;
+ }
+
+ /* Alternate Mode muxes */
+ nval = fwnode_property_read_u16_array(con->fwnode, "svid", NULL, 0);
+ if (nval <= 0)
+ return NULL;
+
+ val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
+ if (!val)
+ return ERR_PTR(-ENOMEM);
+
+ nval = fwnode_property_read_u16_array(con->fwnode, "svid", val, nval);
+ if (nval < 0) {
+ kfree(val);
+ return ERR_PTR(nval);
+ }
+
+ for (i = 0; i < nval; i++) {
+ match = val[i] == desc->svid;
+ if (match) {
+ kfree(val);
+ goto find_mux;
+ }
+ }
+ kfree(val);
+ return NULL;
+
+find_mux:
+ list_for_each_entry(mux, &mux_list, entry)
+ if (dev_fwnode(mux->dev) == con->fwnode)
+ return mux;
+
+ return match ? ERR_PTR(-EPROBE_DEFER) : NULL;
}
/**
--
2.20.1
Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
Hi, On Mon, Feb 11, 2019 at 11:43:17PM +, Michael Hsu wrote: > Hi Heikki, > > > -Original Message- > > From: Heikki Krogerus > > Sent: Friday, February 8, 2019 5:48 AM > > To: Michael Hsu > > Cc: Greg Kroah-Hartman ; Ajay Gupta > > ; linux-usb@vger.kernel.org > > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate > > modes > > > > Hi Michael, > > > > On Thu, Feb 07, 2019 at 10:01:15PM +, Michael Hsu wrote: > > > Hi Heikki, > > > > > > > -Original Message- > > > > From: Heikki Krogerus > > > > Sent: Thursday, February 7, 2019 5:16 AM > > > > To: Michael Hsu > > > > Cc: Greg Kroah-Hartman ; Ajay Gupta > > > > ; linux-usb@vger.kernel.org > > > > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for > > > > alternate modes > > > > > > > > Hi Michael, > > > > > > > > On Wed, Feb 06, 2019 at 10:36:22PM +, Michael Hsu wrote: > > > > > Your existing patch matches the SVID and then requires one-for-one > > > > > ordering in case either alt mode table has multiple mode entries > > > > > for a > > > > particular SVID. > > > > > > > > And we'll fix that, but that does not solve the issue that I'm > > > > talking about. There are two separate issues here. That problem is a > > > > bug in the driver, but the major problem still is that you have > > > > separate alternate modes for the two DP pin assignments, and fixing the > > index handling does not help with that. > > > > > > > > According to the DP alt mode standard you do not have a separate > > > > mode for every supported DṔ pin assignment. When you change the DP > > > > alt mode pin assignment, the current mode is not exited and a new one > > entered. > > > > You stay in the DisplayPort mode, and simply re-configure using the > > > > DisplayPort Configure command. In your implementation however, the > > > > mode is existed, and a new one entered. So your implementation is > > > > not made according to the DisplayPort Alt Mode standard. > > > > > > > > I guess I have been able to explain just how big of a problem that > > > > is, and also how exactly we will have to handle it... Let's look at > > > > this from user space perspective. > > > > > > > > The user space should not need to be aware of the method used to > > > > interface with the USB Type-C connectors on the system. The kernel > > > > needs to hide that and supply unified interface to the user space > > > > which looks and behaves the same, _always_. > > > > > > > > In case of the DP alt mode, the user space should be allowed to > > > > expect the behaviour to follow the DP alt mode spec, so when the > > > > user space selects another pin assignment for the DP alt mode > > > > adapter, it should be a pass-fail operation without any side > > > > effects, just like explained in the VDM flows in the DP alt mode spec. > > > > > > > > But now with your PPM, there is a major side effect. Every time the > > > > user space selects a new DP pin assignment, the current mode is > > > > actually exited and a new mode entered. That is not standard > > > > behaviour. The user space will not just ignore the mode being exited. > > > > > > Not exactly true... If you look on a USB PD analyzer at the type c > > > connector's CC lines, you would *not* see and "Exit Mode" VDMs - just "DP > > Configure" VDMs. > > > > So your PPM does not actually reflect the behaviour of the hardware and > > firmware. You are not actually changing the mode when you change the pin > > assignment, yet you still want the operating system to think it has to > > change the mode every time it changes the pin assignment. > > > > That means the two modes you have are for completely customised software > > (firmware) representation of the DisplayPort alt mode support that does not > > follow the standard, and as we can now see, reflect even the actual > > behaviour. > > > > If your connector could act as the sink, how many DP alt modes would return > > to the partner as a response to Discover Modes with DP SVID? You would > > return only one regardless of how many pin assignments you support, right? > > That's wha
Re: [PATCH v2 0/9] device connection: Add support for device graphs
On Tue, Feb 12, 2019 at 10:44:35AM +, Jun Li wrote: > Hi > > -Original Message- > > From: Heikki Krogerus > > Sent: 2019年1月31日 0:03 > > To: Greg Kroah-Hartman > > Cc: Andy Shevchenko ; Chen Yu > > ; Jun Li ; Hans de Goede > > ; linux-usb@vger.kernel.org; > > linux-ker...@vger.kernel.org > > Subject: [PATCH v2 0/9] device connection: Add support for device graphs > > > > Hi, > > > > This is the second version of this series. On top the two code style > > improvements > > requested by Andy, I also renamed the connection identifiers used with the > > USB > > Type-C muxes for something that I felt are better, especially after we > > start using > > them to name reference device properties in fwnodes. That's why the first > > patch is > > now split in two, 1/9 and 3/9. > > > > Hans! Please note that there is no functional change. The alt mode device > > is still > > getting a handle to the mux, just like before. > > That was actually happening also in the first version of the series. > > > > The commit message from v1: > > > > This series adds support for OF and ACPI device graph parsing to the device > > connection API. > > > > Handling the graph is straightforward, but because I'm adding that fwnode > > member > > to struct device_connection, I had to make sure all the existing users > > consider it. > > > > The plan is to only support matching with fwnode in the future, so no more > > device > > name matching. The software fwnodes that we now have in kernel should make > > that > > possible, once we add support for references to them. > > > > The original RFC: > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org% > > 2Flkml%2F2018%2F10%2F24%2F619&data=02%7C01%7Cjun.li%40nxp.co > > m%7C2fd7c8c28d67434354be08d686cc6b55%7C686ea1d3bc2b4c6fa92cd99c5c > > 301635%7C0%7C0%7C636844609858846167&sdata=AWDD9WaO%2BXxM > > Izlli6GUNEq%2FqUpa5hSyLbBsjICdLIo%3D&reserved=0 > > > > thanks, > > > > Heikki Krogerus (9): > > platform/x86: intel_cht_int33fe: Prepare for better mux naming scheme > > usb: typec: Rationalize the API for the muxes > > platform/x86: intel_cht_int33fe: Remove old style mux connections > > device connection: Add fwnode member to struct device_connection > > usb: typec: mux: Find the muxes by also matching against the device > > node > > usb: roles: Find the muxes by also matching against the device node > > usb: typec: Find the ports by also matching against the device node > > device connection: Prepare support for firmware described connections > > device connection: Find device connections also from device graphs > > > > drivers/base/devcon.c| 62 ++- > > drivers/platform/x86/intel_cht_int33fe.c | 15 ++-- > > drivers/usb/roles/class.c| 21 +- > > drivers/usb/typec/class.c| 31 ++-- > > drivers/usb/typec/mux.c | 96 > > include/linux/device.h | 6 ++ > > include/linux/usb/role.h | 1 + > > include/linux/usb/typec_mux.h| 3 +- > > 8 files changed, 195 insertions(+), 40 deletions(-) > > > > -- > > 2.20.1 > > I tested this series on dwc3+typec, for usb role switch and typec switch > common part > Reviewed-by: Jun Li > Tested-by: Jun Li Thanks Jun. I'm going to send one more version, and fix the description of the struct usb_role_switch_desc like you proposed. Since the fix will only add one comment line to the code, I'm going to take the liberty to add your Reviewed-by and Tested-by tags this time (I don't usually like to carry them to new versions of patches). I'll do the same with the others. thanks, -- heikki
Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node
Hi,
On Tue, Feb 12, 2019 at 10:41:28AM +, Jun Li wrote:
> > @@ -32,6 +32,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct
> > device *dev);
> > * usb_role_switch_register() before registering the switch.
> > */
> > struct usb_role_switch_desc {
> > + struct fwnode_handle *fwnode;
> You may add some description for this new member
> /**
> * struct usb_role_switch_desc - USB Role Switch Descriptor
> * @ fwnode
You are correct. I need to fix that one.
thanks,
--
heikki
Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node
Hi,
On Tue, Feb 12, 2019 at 06:03:42AM +, Jun Li wrote:
> > > return dev_fwnode(dev->parent) == fwnode;
> >
> > That's actually not the case. struct usb_role_switch_desc has a member for
> > fwnode,
> > and that's what we use with the actual mux device. Check
> > usb_role_switch_register():
> >
> > ...
> > sw->dev.fwnode = desc->fwnode;
> > ...
> >
> > Sorry for not realizing it before.
>
> So desc->fwnode should be initialized before do usb_role_switch_register()?
> But seems usb_role_switch_desc is a read-only object so can't be set at
> runtime.
It can. Even though usb_role_switch_register() takes read-only
parameter, nothing's preventing you from passing data even from the
stack (the content of the descriptor is copied in any case).
Expecting the descriptor to be read-only just means it can be
read-only, but it does not have to be.
> usb_controller_node {
> ...
> usb-role-switch;
>
> port {
> sw_provider_node: endpoint {
> remote-endpoint = <&sw_consumer_node>;
> };
> };
> };
>
> typec_node {
> ...
> port {
> sw_consumer_node: endpoint {
> remote-endpoint = <&sw_provider_node>;
> };
> };
> };
That looks roughly correct to me.
thanks,
--
heikki
Re: [PATCH v5 0/2] Add support for firmware update on Cypres CCGx
On Thu, Feb 07, 2019 at 11:18:11AM -0800, Ajay Gupta wrote: > Hi Heikki > > These changes adds support for updating firmware on Cypress CCGx > controller. New version (v5) has fixes from Oliver Neukum > > I have tested them on NVIDIA GPU card. > > Firmware binary pull-request is posted to linux-firmware.git at [1]. > > Please help review this set. > > Thanks > Ajay > > [1] https://marc.info/?l=linux-usb&m=154948878626598&w=2 > > Ajay Gupta (2): > usb: typec: ucsi: add get_fw_info function > usb: typec: ucsi: add firmware flashing support > > drivers/usb/typec/ucsi/ucsi_ccg.c | 884 +- > 1 file changed, 874 insertions(+), 10 deletions(-) These look OK to me. Acked-by: Heikki Krogerus thanks, -- heikki
Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node
On Mon, Feb 11, 2019 at 12:46:29PM +0200, Heikki Krogerus wrote:
> On Mon, Feb 11, 2019 at 09:58:04AM +, Jun Li wrote:
> > Hi Heikki,
> >
> > > @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
> > > usb_role_switch *sw) } EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> > >
> > > -static int __switch_match(struct device *dev, const void *name)
> > > +static int switch_fwnode_match(struct device *dev, const void *fwnode)
> > > +{
> > > + return dev_fwnode(dev) == fwnode;
> >
> > You missed the comment
> > https://lkml.org/lkml/2019/1/22/437
> >
> > return dev_fwnode(dev->parent) == fwnode;
>
> That's actually not the case. struct usb_role_switch_desc has a member
> for fwnode, and that's what we use with the actual mux device. Check
> usb_role_switch_register():
>
> ...
> sw->dev.fwnode = desc->fwnode;
> ...
>
> Sorry for not realizing it before.
Just to clarify. The current patch is OK. No changes needed.
thanks,
--
heikki
Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node
On Mon, Feb 11, 2019 at 09:58:04AM +, Jun Li wrote:
> Hi Heikki,
>
> > @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
> > usb_role_switch *sw) } EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> >
> > -static int __switch_match(struct device *dev, const void *name)
> > +static int switch_fwnode_match(struct device *dev, const void *fwnode)
> > +{
> > + return dev_fwnode(dev) == fwnode;
>
> You missed the comment
> https://lkml.org/lkml/2019/1/22/437
>
> return dev_fwnode(dev->parent) == fwnode;
That's actually not the case. struct usb_role_switch_desc has a member
for fwnode, and that's what we use with the actual mux device. Check
usb_role_switch_register():
...
sw->dev.fwnode = desc->fwnode;
...
Sorry for not realizing it before.
thanks,
--
heikki
Re: [PATCH v2 7/9] usb: typec: Find the ports by also matching against the device node
Hi Andy, On Thu, Jan 31, 2019 at 03:35:37PM +0200, Heikki Krogerus wrote: > On Wed, Jan 30, 2019 at 06:51:56PM +0200, Andy Shevchenko wrote: > > On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus > > wrote: > > > > > > When the connections are defined in firmware, struct > > > device_connection will have the fwnode member pointing to > > > the device node (struct fwnode_handle) of the requested > > > device, and the endpoint will not be used at all in that > > > case. > > > > > + /* > > > +* FIXME: Check does the fwnode supports the requested SVID. If > > > it does > > > +* we need to return ERR_PTR(-PROBE_DEFER) when there is no > > > device. > > > +*/ > > > + if (con->fwnode) > > > + return class_find_device(typec_class, NULL, con->fwnode, > > > +typec_port_fwnode_match); > > > + > > > + dev = class_find_device(typec_class, NULL, con->endpoint[ep], > > > + typec_port_name_match); > > > + > > > + return dev ? dev : ERR_PTR(-EPROBE_DEFER); > > > > Just to be clear, this one takes a reference on dev. Is it taken into > > account? > > Yes. That is what we want it to do. Gentle ping. Is the series OK? thanks, -- heikki
Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
Hi Michael, On Thu, Feb 07, 2019 at 10:01:15PM +, Michael Hsu wrote: > Hi Heikki, > > > -Original Message- > > From: Heikki Krogerus > > Sent: Thursday, February 7, 2019 5:16 AM > > To: Michael Hsu > > Cc: Greg Kroah-Hartman ; Ajay Gupta > > ; linux-usb@vger.kernel.org > > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate > > modes > > > > Hi Michael, > > > > On Wed, Feb 06, 2019 at 10:36:22PM +, Michael Hsu wrote: > > > Your existing patch matches the SVID and then requires one-for-one > > > ordering in case either alt mode table has multiple mode entries for a > > particular SVID. > > > > And we'll fix that, but that does not solve the issue that I'm talking > > about. There > > are two separate issues here. That problem is a bug in the driver, but the > > major > > problem still is that you have separate alternate modes for the two DP pin > > assignments, and fixing the index handling does not help with that. > > > > According to the DP alt mode standard you do not have a separate mode for > > every supported DṔ pin assignment. When you change the DP alt mode pin > > assignment, the current mode is not exited and a new one entered. > > You stay in the DisplayPort mode, and simply re-configure using the > > DisplayPort > > Configure command. In your implementation however, the mode is existed, > > and a new one entered. So your implementation is not made according to the > > DisplayPort Alt Mode standard. > > > > I guess I have been able to explain just how big of a problem that is, and > > also > > how exactly we will have to handle it... Let's look at this from user space > > perspective. > > > > The user space should not need to be aware of the method used to interface > > with the USB Type-C connectors on the system. The kernel needs to hide that > > and supply unified interface to the user space which looks and behaves the > > same, _always_. > > > > In case of the DP alt mode, the user space should be allowed to expect the > > behaviour to follow the DP alt mode spec, so when the user space selects > > another pin assignment for the DP alt mode adapter, it should be a pass-fail > > operation without any side effects, just like explained in the VDM flows in > > the > > DP alt mode spec. > > > > But now with your PPM, there is a major side effect. Every time the user > > space > > selects a new DP pin assignment, the current mode is actually exited and a > > new > > mode entered. That is not standard behaviour. The user space will not just > > ignore the mode being exited. > > Not exactly true... If you look on a USB PD analyzer at the type c connector's > CC lines, you would *not* see and "Exit Mode" VDMs - just "DP Configure" VDMs. So your PPM does not actually reflect the behaviour of the hardware and firmware. You are not actually changing the mode when you change the pin assignment, yet you still want the operating system to think it has to change the mode every time it changes the pin assignment. That means the two modes you have are for completely customised software (firmware) representation of the DisplayPort alt mode support that does not follow the standard, and as we can now see, reflect even the actual behaviour. If your connector could act as the sink, how many DP alt modes would return to the partner as a response to Discover Modes with DP SVID? You would return only one regardless of how many pin assignments you support, right? That's what you need to return to the OS as well, as source or sink. > The "active" sysfs node with the value of "yes" would change paths (since > another UCSI CAM index is active), but that's because there was a UCSI > connector status change. > > > It has to react to it, most likely by assuming there was a fatal error > > somewhere. > > So in practice the user space is now forced to handle your connector as a > > special case. > > The expected user behavior is to read every sysfs node of the form... >/sys/class/typec/.../svid >/sys/class/typec/.../vdo > And if it decides that's the alt mode it wants to activate, it writes 1 to >/sys/class/typec/.../active > And optionally, reads above sysfs file to confirm that it did indeed change to > 'yes'. > It would treat it as an failure to enter alt mode if it was read as still > 'no'. Do you understand that those steps you can take _only_ on your hardware in order to change the DP pin assignment? They work only with your PPM. The ap
Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
Hi Michael, On Wed, Feb 06, 2019 at 10:36:22PM +, Michael Hsu wrote: > Your existing patch matches the SVID and then requires one-for-one ordering in > case either alt mode table has multiple mode entries for a particular SVID. And we'll fix that, but that does not solve the issue that I'm talking about. There are two separate issues here. That problem is a bug in the driver, but the major problem still is that you have separate alternate modes for the two DP pin assignments, and fixing the index handling does not help with that. According to the DP alt mode standard you do not have a separate mode for every supported DṔ pin assignment. When you change the DP alt mode pin assignment, the current mode is not exited and a new one entered. You stay in the DisplayPort mode, and simply re-configure using the DisplayPort Configure command. In your implementation however, the mode is existed, and a new one entered. So your implementation is not made according to the DisplayPort Alt Mode standard. I guess I have been able to explain just how big of a problem that is, and also how exactly we will have to handle it... Let's look at this from user space perspective. The user space should not need to be aware of the method used to interface with the USB Type-C connectors on the system. The kernel needs to hide that and supply unified interface to the user space which looks and behaves the same, _always_. In case of the DP alt mode, the user space should be allowed to expect the behaviour to follow the DP alt mode spec, so when the user space selects another pin assignment for the DP alt mode adapter, it should be a pass-fail operation without any side effects, just like explained in the VDM flows in the DP alt mode spec. But now with your PPM, there is a major side effect. Every time the user space selects a new DP pin assignment, the current mode is actually exited and a new mode entered. That is not standard behaviour. The user space will not just ignore the mode being exited. It has to react to it, most likely by assuming there was a fatal error somewhere. So in practice the user space is now forced to handle your connector as a special case. To protect the user space from that, and to keep the interface we provide to it consistent and predictable, we would have to handle your PPM completely separately. The user space will see only one connector DP alt mode no matter what. Even if you are unable change the PPM, the driver will still have to expose the two connector DP alternate modes as one connector alternate mode to the user space. That does not leave much use for the separated modes for the two pin assignments. The only thing they are providing in any case is the guarantee of the initial pin assignment, but as said, that we can guess in any case. Sorry for the long explanation. Br, -- heikki
Re: [PATCH v3 0/6] Add support for firmware update on Cypres CCGx
Hi Ajay, On Fri, Feb 01, 2019 at 05:05:04PM -0800, Ajay Gupta wrote: > Hi Heikki > > These changes adds support for updating firmware on Cypress CCGx > controller. New version (v3) fixes comments form you and Greg. > > I have tested them on NVIDIA GPU card. > > I will be posting firmware binary patch to linux-firmware.git repo soon. > > Please help review this set. > > Thanks > Ajay > > Ajay Gupta (6): > usb: typec: ucsi: add get_fw_info function > usb: typec: ucsi: add ccg command framework > usb: typec: ucsi: add port num info > usb: typec: ucsi: add cmd used for fw flashing > usb: typec: ucsi: add fw update needed check > usb: typec: ucsi: add firmware flashing support > > drivers/usb/typec/ucsi/ucsi_ccg.c | 881 +- > 1 file changed, 874 insertions(+), 7 deletions(-) I think in this case you can add this support in a single patch. Br, -- heikki
Re: [PATCH v3 4/6] usb: typec: ucsi: add cmd used for fw flashing
On Fri, Feb 01, 2019 at 05:05:08PM -0800, Ajay Gupta wrote:
> From: Ajay Gupta
>
> Adding support for below commands which will be used
> during firmware flashing.
> - ENTER_FLASHING
> - RESET
> - PDPORT_ENABLE
> - JUMP_TO_BOOT
> - FLASH_ROW_RW
> - VALIDATE_FW
> I command specific mutex lock is also added to sync
> between driver and user threads.
>
> Signed-off-by: Ajay Gupta
> ---
> Changes from v2 to v3
> - None
>
> drivers/usb/typec/ucsi/ucsi_ccg.c | 216 ++
> 1 file changed, 216 insertions(+)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 63b07b7d17f2..b9bbe90bdf57 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -30,13 +30,34 @@ enum enum_fw_mode {
> #define PORT0_INT BIT(1)
> #define PORT1_INT BIT(2)
> #define UCSI_READ_INT BIT(7)
> +#define CCGX_RAB_JUMP_TO_BOOT0x0007
> +#define TO_BOOT 'J'
> +#define TO_ALT_FW 'A'
> +#define CCGX_RAB_RESET_REQ 0x0008
> +#define RESET_SIG 'R'
> +#define CMD_RESET_I2C 0x0
> +#define CMD_RESET_DEV 0x1
> +#define CCGX_RAB_ENTER_FLASHING 0x000A
> +#define FLASH_ENTER_SIG 'P'
> +#define CCGX_RAB_VALIDATE_FW 0x000B
> +#define CCGX_RAB_FLASH_ROW_RW0x000C
> +#define FLASH_SIG 'F'
> +#define FLASH_RD_CMD0x0
> +#define FLASH_WR_CMD0x1
> +#define FLASH_FWCT1_WR_CMD 0x2
> +#define FLASH_FWCT2_WR_CMD 0x3
> +#define FLASH_FWCT_SIG_WR_CMD 0x4
> #define CCGX_RAB_READ_ALL_VER0x0010
> #define CCGX_RAB_READ_FW2_VER0x0020
> #define CCGX_RAB_UCSI_CONTROL0x0039
> #define CCGX_RAB_UCSI_CONTROL_START BIT(0)
> #define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
> #define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
> +#define REG_FLASH_RW_MEM0x0200
> #define DEV_REG_IDX CCGX_RAB_DEVICE_MODE
> +#define CCGX_RAB_PDPORT_ENABLE 0x002C
> +#define PDPORT_1BIT(0)
> +#define PDPORT_2BIT(1)
> #define CCGX_RAB_RESPONSE0x007E
> #define ASYNC_EVENT BIT(7)
>
> @@ -47,6 +68,13 @@ enum enum_fw_mode {
> #define PORT_DISCONNECT_DET 0x85
> #define ROLE_SWAP_COMPELETE 0x87
>
> +/* ccg firmware */
> +#define CYACD_LINE_SIZE 527
> +#define CCG4_ROW_SIZE 256
> +#define FW1_METADATA_ROW0x1FF
> +#define FW2_METADATA_ROW0x1FE
> +#define FW_CFG_TABLE_SIG_SIZE256
> +
> struct ccg_dev_info {
> #define CCG_DEVINFO_FWMODE_SHIFT (0)
> #define CCG_DEVINFO_FWMODE_MASK (0x3 << CCG_DEVINFO_FWMODE_SHIFT)
> @@ -118,6 +146,7 @@ struct ucsi_ccg {
> struct ccg_resp dev_resp;
> u8 cmd_resp;
> int port_num;
> + struct mutex lock; /* to sync between user and driver thread */
> };
>
> static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> @@ -431,6 +460,193 @@ static int ccg_send_command(struct ucsi_ccg *uc, struct
> ccg_cmd *cmd)
> return uc->cmd_resp;
> }
>
> +static int ccg_cmd_enter_flashing(struct ucsi_ccg *uc)
> +{
> + struct ccg_cmd cmd;
> + int ret;
> +
> + cmd.reg = CCGX_RAB_ENTER_FLASHING;
> + cmd.data = FLASH_ENTER_SIG;
> + cmd.len = 1;
> + cmd.delay = 50;
> +
> + mutex_lock(&uc->lock);
> +
> + ret = ccg_send_command(uc, &cmd);
> +
> + mutex_unlock(&uc->lock);
> +
> + if (ret != CMD_SUCCESS) {
> + dev_err(uc->dev, "enter flashing failed ret=%d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ccg_cmd_reset(struct ucsi_ccg *uc, bool extra_delay)
> +{
> + struct ccg_cmd cmd;
> + u8 *p;
> + int ret;
> +
> + p = (u8 *)&cmd.data;
> + cmd.reg = CCGX_RAB_RESET_REQ;
> + p[0] = RESET_SIG;
> + p[1] = CMD_RESET_DEV;
> + cmd.len = 2;
> + cmd.delay = 2000 + (extra_delay ? 3000 : 0);
> +
> + mutex_lock(&uc->lock);
> +
> + set_bit(RESET_PENDING, &uc->flags);
> +
> + ret = ccg_send_command(uc, &cmd);
> + if (ret != RESET_COMPLETE)
> + goto err_clear_flag;
> +
> + ret = 0;
> +
> +err_clear_flag:
> + clear_bit(RESET_PENDING, &uc->flags);
> +
> + mutex_unlock(&uc->lock);
> +
> + return ret;
> +}
> +
> +static int ccg_cmd_port_control(struct ucsi_ccg *uc, bool enable)
> +{
> + struct ccg_cmd cmd;
> + int ret;
> +
> + cmd.reg = CCGX_RAB_PDPORT_ENABLE;
>
Re: [PATCH v3 2/6] usb: typec: ucsi: add ccg command framework
Hi Ajay,
Sorry for the late response, but I hit a compiler warning after doing
bisect test.
On Fri, Feb 01, 2019 at 05:05:06PM -0800, Ajay Gupta wrote:
> +/* CCGx response codes */
> +enum ccg_resp_code {
> + CMD_NO_RESP = 0x00,
> + CMD_SUCCESS = 0x02,
> + FLASH_DATA_AVAILABLE= 0x03,
> + CMD_INVALID = 0x05,
> + FLASH_UPDATE_FAIL = 0x07,
> + INVALID_FW = 0x08,
> + INVALID_ARG = 0x09,
> + CMD_NOT_SUPPORT = 0x0A,
> + TRANSACTION_FAIL= 0x0C,
> + PD_CMD_FAIL = 0x0D,
> + UNDEF_ERROR = 0x0F,
> + INVALID_RESP= 0x10,
> +};
Side note. There are no users for these, so I'm not sure you need to
add them in this patch.
> +#define CCG_EVENT_MAX(EVENT_INDEX + 43)
> +
> +struct ccg_cmd {
> + u16 reg;
> + u32 data;
> + int len;
> + int delay; /* ms delay for cmd timeout */
> +};
> +
> +struct ccg_resp {
> + u8 code;
> + u8 length;
> +};
> +
> struct ucsi_ccg {
> struct device *dev;
> struct ucsi *ucsi;
> @@ -58,17 +111,14 @@ struct ucsi_ccg {
> struct ccg_dev_info info;
> /* version info for boot, primary and secondary */
> struct version_info version[FW2 + 1];
> + /* CCG HPI communication flags */
> + unsigned long flags;
> +#define RESET_PENDING0
> +#define DEV_CMD_PENDING 1
> + struct ccg_resp dev_resp;
> + u8 cmd_resp;
> };
>
> -#define CCGX_RAB_DEVICE_MODE 0x
> -#define CCGX_RAB_INTR_REG0x0006
> -#define CCGX_RAB_READ_ALL_VER0x0010
> -#define CCGX_RAB_READ_FW2_VER0x0020
> -#define CCGX_RAB_UCSI_CONTROL0x0039
> -#define CCGX_RAB_UCSI_CONTROL_START BIT(0)
> -#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
> -#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
> -
> static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> {
> struct i2c_client *client = uc->client;
> @@ -276,6 +326,110 @@ static int get_fw_info(struct ucsi_ccg *uc)
> return 0;
> }
>
> +static inline bool invalid_async_evt(int code)
> +{
> + return (code >= CCG_EVENT_MAX) || (code < EVENT_INDEX);
> +}
> +
> +static void ccg_process_response(struct ucsi_ccg *uc)
> +{
> + struct device *dev = uc->dev;
> +
> + if (uc->dev_resp.code & ASYNC_EVENT) {
> + if (uc->dev_resp.code == RESET_COMPLETE) {
> + if (test_bit(RESET_PENDING, &uc->flags))
> + uc->cmd_resp = uc->dev_resp.code;
> + get_fw_info(uc);
> + }
> + if (invalid_async_evt(uc->dev_resp.code))
> + dev_err(dev, "invalid async evt %d\n",
> + uc->dev_resp.code);
> + } else {
> + if (test_bit(DEV_CMD_PENDING, &uc->flags)) {
> + uc->cmd_resp = uc->dev_resp.code;
> + clear_bit(DEV_CMD_PENDING, &uc->flags);
> + } else {
> + dev_err(dev, "dev resp 0x%04x but no cmd pending\n",
> + uc->dev_resp.code);
> + }
> + }
> +}
> +
> +static int ccg_read_response(struct ucsi_ccg *uc)
> +{
> + unsigned long target = jiffies + msecs_to_jiffies(1000);
> + struct device *dev = uc->dev;
> + u8 intval;
> + int status;
> +
> + /* wait for interrupt status to get updated */
> + do {
> + status = ccg_read(uc, CCGX_RAB_INTR_REG, &intval,
> + sizeof(intval));
> + if (status < 0)
> + return status;
> +
> + if (intval & DEV_INT)
> + break;
> + usleep_range(500, 600);
> + } while (time_is_after_jiffies(target));
> +
> + if (time_is_before_jiffies(target)) {
> + dev_err(dev, "response timeout error\n");
> + return -ETIME;
> + }
> +
> + status = ccg_read(uc, CCGX_RAB_RESPONSE, (u8 *)&uc->dev_resp,
> + sizeof(uc->dev_resp));
> + if (status < 0)
> + return status;
> +
> + status = ccg_write(uc, CCGX_RAB_INTR_REG, &intval, sizeof(intval));
> + if (status < 0)
> + return status;
> +
> + return 0;
> +}
> +
> +/* Caller must hold uc->lock */
> +static int ccg_send_command(struct ucsi_ccg *uc, struct ccg_cmd *cmd)
> +{
> + struct device *dev = uc->dev;
> + int ret;
> +
> + switch (cmd->reg & 0xF000) {
> + case DEV_REG_IDX:
> + set_bit(DEV_CMD_PENDING, &uc->flags);
> + break;
> + default:
> + dev_err(dev, "invalid cmd register\n");
> + break;
> + }
> +
> + ret = ccg_write(uc, cmd->reg, (u8 *)&cmd->data, cmd->len);
> + if (ret < 0)
> + return ret;
> +
> + msleep(cmd->delay);
> +
> +
Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
Hi Michael, On Fri, Feb 01, 2019 at 10:02:19PM +, Michael Hsu wrote: > Hi Heikki, the use of "con->port_altmode[cur]->mode" (which is a 1-based > index, not a 32-bit mode VDO) can cause incorrect matches if the > GET_ALTERNATE_MODES returns different ordering for recipient=connector and > recipient=sop for a particular svid. > > For example, UCSI command GET_ALTERNATE_MODES with recipient=connector returns > [0] SVID=0xff01, ModeVDO=0x0405 (Mode = 1) > [1] SVID=0xff01, ModeVDO=0x0805 (Mode = 2) > And UCSI command GET_ALTERNATE_MODES with recipient=sop returns > [0] SVID=0xff01, ModeVDO=0x0c05 (Mode = 1) > > Then when DP alternate mode with pin D is active, GET_CURRENT_CAM returns > index 1 (connector alternate mode = 1, i.e. SVID=0xff01, Mode=2, > ModeVDO=0x0805). > But, the function will be unable to match it with partner_alt_mode > corresponding to (SVID=0xff01,Mode=1). > > Can you compare against 32-bit VDO instead of ->mode? Also, use '&' bitwise > AND operator when masking the partner's mode VDO (0x0c05) against the > connector's mode VDO (0x405 or 0x805) to determine it there is an alternate > mode match. FYI, I'll improve the matching done ucsi_altmode_update_active(), it is true that we cannot rely on the the mode index, but note I'm not going to support those masks at this point. I'm still expecting that the firmware can be updated. The UCSI data structures really need to supply consistent information to the OS, regardless of the PPM implementation. I threw some ideas how we should be able to determine the DP pin assignment with more certainty in my previous mail, in case the currently executed "guessing" is not reliable enough for you guys. Let me know what you think. thanks, -- heikki
Re: [PATCH 5/5] usb: typec: ucsi: Support for DisplayPort alt mode
Hi Ajay,
On Tue, Feb 05, 2019 at 07:24:49PM +, Ajay Gupta wrote:
> > +static int ucsi_displayport_configure(struct ucsi_dp *dp) {
> > + u32 pins = DP_CONF_GET_PIN_ASSIGN(dp->data.conf);
> > + struct ucsi_control ctrl;
> > +
> > + if (!dp->override)
> > + return 0;
> > +
> > + ctrl.raw_cmd = UCSI_CMD_SET_NEW_CAM(dp->con->num, 1, dp-
> > >offset,
> > +pins);
> If previous CAM is active at this point then we should exit before setting new
> CAM. Please add something like :
>
> + if (!dp->override)
> + return 0;
>
> UCSI_CMD_GET_CURRENT_CAM(ctrl, dp->con->num);
> ret = ucsi_send_command(dp->con->ucsi, &ctrl, &cur, sizeof(cur));
> while ((cur != 0xff) && (cur != dp->offset)) {
> ctrl.raw_cmd = UCSI_CMD_SET_NEW_CAM(dp->con->num, 0, cur, 0);
> ret = ucsi_send_command(dp->con->ucsi, &ctrl, NULL, 0);
> UCSI_CMD_GET_CURRENT_CAM(ctrl, dp->con->num);
> ret = ucsi_send_command(dp->con->ucsi, &ctrl, &cur,
> sizeof(cur));
> }
> + ctrl.raw_cmd = UCSI_CMD_SET_NEW_CAM(dp->con->num, 1, dp-
I'll take a look at this before preparing the next version.
thanks,
--
heikki
Re: [PATCH 5/5] usb: typec: ucsi: Support for DisplayPort alt mode
Hi,
On Tue, Feb 05, 2019 at 01:03:58AM +, Ajay Gupta wrote:
> > @@ -412,6 +424,12 @@ static void ucsi_unregister_altmodes(struct
> > ucsi_connector *con, u8 recipient)
> > }
> >
> > while (adev[i]) {
> > + if (recipient == UCSI_RECIPIENT_SOP &&
> > + adev[i]->svid == USB_TYPEC_DP_SID) {
> > + alt = typec_match_altmode(con->port_altmode, -1,
> > + USB_TYPEC_DP_SID, 1);
> > + ucsi_displayport_remove_partner(alt);
> "alt" may be null here in cases where attached type-C device has DP alt mode
> which is not supported
> by connector. Please change as
> If (alt)
> ucsi_displayport_remove_partner(alt);
That should not be necessary. Check ucsi_displayport_remove_partner().
I put a condition there if (!alt) return;
thanks,
--
heikki
Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
Hi Ajay,
On Tue, Feb 05, 2019 at 12:59:27AM +, Ajay Gupta wrote:
> > +static int ucsi_register_altmode(struct ucsi_connector *con,
> > +struct typec_altmode_desc *desc,
> > +u8 recipient)
> > +{
> > + struct typec_altmode *alt;
> > + int ret;
> > + int i;
> > +
> > + switch (recipient) {
> > + case UCSI_RECIPIENT_CON:
> > + i = ucsi_next_altmode(con->port_altmode);
> > + if (i < 0) {
> > + ret = i;
> > + goto err;
> > + }
> > +
> > + desc->mode = ucsi_altmode_next_mode(con->port_altmode,
> > + desc->svid);
> > +
> > + alt = typec_port_register_altmode(con->port, desc);
> > + if (IS_ERR(alt)) {
> > + ret = PTR_ERR(alt);
> > + goto err;
> > + }
> > +
> > + con->port_altmode[i] = alt;
> > + break;
> > + case UCSI_RECIPIENT_SOP:
> > + i = ucsi_next_altmode(con->partner_altmode);
> We are seeing duplicate partner altmode devices getting created when we set
> "active" file from 1->0->1 Please add a check here to see if altmode device
> already exists.
>
> [...]
> case UCSI_RECIPIENT_SOP:
> /* check to see if partner altmode already exists */
> if (ucsi_altmode_found(con->partner_altmode, desc))
> break;
>
> i = ucsi_next_altmode(con->partner_altmode);
>if (i < 0) {
> [...]
>
>
> static bool ucsi_altmode_found(struct typec_altmode **alt,
>struct typec_altmode_desc *desc)
> {
> int i;
>
> for (i = 0; i < UCSI_MAX_ALTMODES; i++) {
> if (!alt[i])
> return false;
> if (alt[i]->svid == desc->svid && alt[i]->vdo == desc->vdo)
> return true;
> }
>
> return false;
> }
OK. I'll prepare new version later this week.
thanks,
--
heikki
