[PATCH v2 0/3] Add Intel Input Output Manager driver

[PATCH v2 0/3] Add Intel Input Output Manager driver

[Rajmohan Mani]

Hi,

This patch series add support for Intel Input Output Manager (IOM)
driver, which is leveraged by the Intel PMC USB mux control driver.

changes from v1:
- Included device role (UFP) support patch from Heikki to this series.
- Addressed Greg's comments on v1 with below changes
- Used per-instance variable for struct intel_iom
- Removed struct device *dev inside probe function
- Removed dev_set_drvdata() inside probe function
- Added NULL pointer check on struct intel_iom * as well as members
  dev and regbar, inside intel_iom_port_status(), to protect against
  calls to this API before driver gets probed.
- Limit support to only one IOM device
- Removed intel_iom_get()/intel_iom_put() calls, as the dependency through
  Kconfig is enough to prevent the IOM driver from unloading, when used
  by callers.

Heikki Krogerus (2):
  usb: typec: intel_pmc_mux: Check the port status before connect
  usb: typec: intel_pmc_mux: Support for device role (UFP)

Rajmohan Mani (1):
  platform/x86: Add Intel Input Output Manager (IOM) driver

 drivers/platform/x86/Kconfig                |  16 +++
 drivers/platform/x86/Makefile               |   1 +
 drivers/platform/x86/intel_iom.c            |  95 +++++++++++++++
 drivers/usb/typec/mux/Kconfig               |   1 +
 drivers/usb/typec/mux/intel_pmc_mux.c       | 124 ++++++++++++++------
 include/linux/platform_data/x86/intel_iom.h |  49 ++++++++
 6 files changed, 251 insertions(+), 35 deletions(-)
 create mode 100644 drivers/platform/x86/intel_iom.c
 create mode 100644 include/linux/platform_data/x86/intel_iom.h

-- 
2.20.1

[PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Rajmohan Mani]

Input Output Manager (IOM) is part of the Tiger Lake SoC that
configures the Type-C Sub System (TCSS). IOM is a micro controller
that handles Type-C topology, configuration and PM functions of
various Type-C devices connected on the platform.

This driver helps read relevant information such as Type-C port
status (whether a device is connected to a Type-C port or not) and
the activity type on the Type-C ports (such as USB, Display Port,
Thunderbolt), for consumption by other drivers.

Currently intel_iom_port_status() API is exported by this driver,
that has information about the Type-C port status and port activity
type.

Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/platform/x86/Kconfig                | 16 ++++
 drivers/platform/x86/Makefile               |  1 +
 drivers/platform/x86/intel_iom.c            | 95 +++++++++++++++++++++
 include/linux/platform_data/x86/intel_iom.h | 49 +++++++++++
 4 files changed, 161 insertions(+)
 create mode 100644 drivers/platform/x86/intel_iom.c
 create mode 100644 include/linux/platform_data/x86/intel_iom.h

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 40219bba6801..f09b4b525ea5 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -837,6 +837,22 @@ config INTEL_INT0002_VGPIO
 	  To compile this driver as a module, choose M here: the module will
 	  be called intel_int0002_vgpio.
 
+config INTEL_IOM
+	tristate "Intel Input Output Manager (IOM) driver"
+	depends on ACPI && PCI
+	help
+	  This driver helps read relevant information such as Type-C port
+	  status (whether a device is connected to a Type-C port or not)
+	  and the activity type on the Type-C ports (such as USB, Display
+	  Port, Thunderbolt), for consumption by other drivers.
+
+	  Currently intel_iom_port_status() API is exported by this driver,
+	  that has information about the Type-C port status and port activity
+	  type.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called intel_iom.
+
 config INTEL_MENLOW
 	tristate "Thermal Management driver for Intel menlow platform"
 	depends on ACPI_THERMAL
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5f823f7eff45..b44f706074c3 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -77,6 +77,7 @@ intel_cht_int33fe-objs			:= intel_cht_int33fe_common.o \
 					   intel_cht_int33fe_microb.o
 obj-$(CONFIG_INTEL_HID_EVENT)		+= intel-hid.o
 obj-$(CONFIG_INTEL_INT0002_VGPIO)	+= intel_int0002_vgpio.o
+obj-$(CONFIG_INTEL_IOM)			+= intel_iom.o
 obj-$(CONFIG_INTEL_MENLOW)		+= intel_menlow.o
 obj-$(CONFIG_INTEL_OAKTRAIL)		+= intel_oaktrail.o
 obj-$(CONFIG_INTEL_VBTN)		+= intel-vbtn.o
diff --git a/drivers/platform/x86/intel_iom.c b/drivers/platform/x86/intel_iom.c
new file mode 100644
index 000000000000..cda7716410c6
--- /dev/null
+++ b/drivers/platform/x86/intel_iom.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Core SoC Input Output Manager (IOM) driver.
+ *
+ * This driver provides access to the Input Output Manager (IOM) (that
+ * is part of Tiger Lake SoC) registers that can be used to know about
+ * Type-C Sub System related information (such as Type-C port status,
+ * activity type on Type-C ports).
+ *
+ * Copyright (C) 2020, Intel Corporation
+ * Author: Rajmohan Mani <rajmohan.mani@intel.com>
+ */
+
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_data/x86/intel_iom.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define IOM_PORT_STATUS_OFFSET				0x560
+
+struct intel_iom {
+	struct device *dev;
+	void __iomem *regbar;
+};
+
+static struct intel_iom *iom;
+
+/**
+ * intel_iom_port_status() - Get status bits for the Type-C port
+ * @port: Type-C port number
+ * @status: pointer to receive the status bits
+ *
+ * Returns 0 on success, error otherwise.
+ */
+int intel_iom_port_status(u8 port, u32 *status)
+{
+	void __iomem *reg;
+
+	if (!iom || !iom->dev || !iom->regbar)
+		return -ENODEV;
+
+	if (!status || (port > IOM_MAX_PORTS - 1))
+		return -EINVAL;
+
+	reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN * port;
+
+	*status = ioread32(reg);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_iom_port_status);
+
+static int intel_iom_probe(struct platform_device *pdev)
+{
+	void __iomem *addr;
+
+	/* only one IOM device is supported */
+	if (iom)
+		return -EBUSY;
+
+	iom = devm_kzalloc(&pdev->dev, sizeof(*iom), GFP_KERNEL);
+	if (!iom)
+		return -ENOMEM;
+
+	addr = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(addr))
+		return PTR_ERR(addr);
+
+	iom->regbar = addr;
+	iom->dev = &pdev->dev;
+
+	return 0;
+}
+
+static const struct acpi_device_id intel_iom_acpi_ids[] = {
+	{ "INTC1072" },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, intel_iom_acpi_ids);
+
+static struct platform_driver intel_iom_driver = {
+	.probe = intel_iom_probe,
+	.driver = {
+		.name = "intel_iom",
+		.acpi_match_table = intel_iom_acpi_ids,
+	},
+};
+
+module_platform_driver_probe(intel_iom_driver, intel_iom_probe);
+
+MODULE_AUTHOR("Rajmohan Mani <rajmohan.mani@intel.com>");
+MODULE_DESCRIPTION("Intel IOM driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/x86/intel_iom.h b/include/linux/platform_data/x86/intel_iom.h
new file mode 100644
index 000000000000..e4c9a305e7a9
--- /dev/null
+++ b/include/linux/platform_data/x86/intel_iom.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _PLATFORM_DATA_X86_INTEL_IOM_H_
+#define _PLATFORM_DATA_X86_INTEL_IOM_H_
+
+#include <linux/bits.h>
+#include <linux/types.h>
+
+/* Input Output Manager (IOM) PORT STATUS */
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_MASK		GENMASK(9, 6)
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_SHIFT		0x06
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_USB		0x03
+/* activity type: Safe Mode */
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_SAFE_MODE		0x04
+/* activity type: Display Port */
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_DP		0x05
+/* activity type: Display Port Multi Function Device */
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_DP_MFD		0x06
+/* activity type: Thunderbolt */
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_TBT		0x07
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_ALT_MODE_USB	0x0c
+#define IOM_PORT_STATUS_ACTIVITY_TYPE_ALT_MODE_TBT_USB	0x0d
+/* Upstream Facing Port Information */
+#define IOM_PORT_STATUS_UFP				BIT(10)
+/* Display Port Hot Plug Detect status */
+#define IOM_PORT_STATUS_DHPD_HPD_STATUS_MASK		GENMASK(13, 12)
+#define IOM_PORT_STATUS_DHPD_HPD_STATUS_SHIFT		0x0c
+#define IOM_PORT_STATUS_DHPD_HPD_STATUS_ASSERT		0x01
+#define IOM_PORT_STATUS_DHPD_HPD_SOURCE_TBT		BIT(14)
+#define IOM_PORT_STATUS_CONNECTED			BIT(31)
+
+#define IOM_MAX_PORTS					4
+/* Register length in bytes */
+#define IOM_REG_LEN					4
+
+#ifdef CONFIG_ACPI
+
+int intel_iom_port_status(u8 port, u32 *status);
+
+#else
+
+int intel_iom_port_status(struct intel_iom *iom, u8 port, u32 *status)
+{
+	return 0;
+}
+
+#endif
+
+#endif /* _PLATFORM_DATA_X86_INTEL_IOM_H_ */
-- 
2.20.1

[PATCH v2 2/3] usb: typec: intel_pmc_mux: Check the port status before connect

[Rajmohan Mani]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

The PMC microcontroller that we use for configuration, does
not supply any status information back. For port status we
need to talk to another controller on the board called IOM
(I/O manager).

By checking the port status before configuring the muxes, we
can make sure that we do not reconfigure the port after
bootup when the system firmware (for example BIOS) has
already configured it.

Using the status information also to check if DisplayPort
HPD is still asserted when the cable plug is disconnected,
and clearing it if it is.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/usb/typec/mux/Kconfig         |  1 +
 drivers/usb/typec/mux/intel_pmc_mux.c | 75 +++++++++++++++++++++++----
 2 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index a4dbd11f8ee2..46f5512de63d 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -12,6 +12,7 @@ config TYPEC_MUX_PI3USB30532
 config TYPEC_MUX_INTEL_PMC
 	tristate "Intel PMC mux control"
 	depends on INTEL_SCU_IPC
+	depends on INTEL_IOM
 	select USB_ROLE_SWITCH
 	help
 	  Driver for USB muxes controlled by Intel PMC FW. Intel PMC FW can
diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index e4021e13af40..c758d6113c41 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -9,6 +9,7 @@
 #include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/platform_data/x86/intel_iom.h>
 #include <linux/property.h>
 #include <linux/usb/pd.h>
 #include <linux/usb/role.h>
@@ -83,10 +84,22 @@ enum {
 #define PMC_USB_DP_HPD_LVL		BIT(4)
 #define PMC_USB_DP_HPD_IRQ		BIT(5)
 
+/* IOM Port Status */
+#define IOM_PORT_ACTIVITY_IS(_status_, _type_)				\
+	((((_status_) & IOM_PORT_STATUS_ACTIVITY_TYPE_MASK) >>		\
+	  IOM_PORT_STATUS_ACTIVITY_TYPE_SHIFT) ==			\
+	 (IOM_PORT_STATUS_ACTIVITY_TYPE_##_type_))
+
+#define IOM_PORT_HPD_ASSERTED(_status_)					\
+	((((_status_) & IOM_PORT_STATUS_DHPD_HPD_STATUS_MASK) >>	\
+	  IOM_PORT_STATUS_DHPD_HPD_STATUS_SHIFT) &			\
+	 IOM_PORT_STATUS_DHPD_HPD_STATUS_ASSERT)
+
 struct pmc_usb;
 
 struct pmc_usb_port {
 	int num;
+	u32 iom_status;
 	struct pmc_usb *pmc;
 	struct typec_mux *typec_mux;
 	struct typec_switch *typec_sw;
@@ -145,18 +158,17 @@ static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
 }
 
 static int
-pmc_usb_mux_dp_hpd(struct pmc_usb_port *port, struct typec_mux_state *state)
+pmc_usb_mux_dp_hpd(struct pmc_usb_port *port, struct typec_displayport_data *dp)
 {
-	struct typec_displayport_data *data = state->data;
 	u8 msg[2] = { };
 
 	msg[0] = PMC_USB_DP_HPD;
 	msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 
-	if (data->status & DP_STATUS_IRQ_HPD)
+	if (dp->status & DP_STATUS_IRQ_HPD)
 		msg[1] = PMC_USB_DP_HPD_IRQ;
 
-	if (data->status & DP_STATUS_HPD_STATE)
+	if (dp->status & DP_STATUS_HPD_STATE)
 		msg[1] |= PMC_USB_DP_HPD_LVL;
 
 	return pmc_usb_command(port, msg, sizeof(msg));
@@ -169,8 +181,18 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
 	struct altmode_req req = { };
 	int ret;
 
+	if (IOM_PORT_ACTIVITY_IS(port->iom_status, DP) ||
+	    IOM_PORT_ACTIVITY_IS(port->iom_status, DP_MFD)) {
+		if (IOM_PORT_HPD_ASSERTED(port->iom_status) &&
+		    (!(data->status & DP_STATUS_IRQ_HPD) &&
+		    data->status & DP_STATUS_HPD_STATE))
+			return 0;
+
+		return pmc_usb_mux_dp_hpd(port, state->data);
+	}
+
 	if (data->status & DP_STATUS_IRQ_HPD)
-		return pmc_usb_mux_dp_hpd(port, state);
+		return pmc_usb_mux_dp_hpd(port, state->data);
 
 	req.usage = PMC_USB_ALT_MODE;
 	req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
@@ -192,8 +214,8 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
 	if (ret)
 		return ret;
 
-	if (data->status & DP_STATUS_HPD_STATE)
-		return pmc_usb_mux_dp_hpd(port, state);
+	if (data->status & (DP_STATUS_IRQ_HPD | DP_STATUS_HPD_STATE))
+		return pmc_usb_mux_dp_hpd(port, state->data);
 
 	return 0;
 }
@@ -205,6 +227,10 @@ pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
 	u8 cable_speed = TBT_CABLE_SPEED(data->cable_mode);
 	struct altmode_req req = { };
 
+	if (IOM_PORT_ACTIVITY_IS(port->iom_status, TBT) ||
+	    IOM_PORT_ACTIVITY_IS(port->iom_status, ALT_MODE_TBT_USB))
+		return 0;
+
 	req.usage = PMC_USB_ALT_MODE;
 	req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 	req.mode_type = PMC_USB_MODE_TYPE_TBT << PMC_USB_MODE_TYPE_SHIFT;
@@ -239,6 +265,10 @@ pmc_usb_mux_usb4(struct pmc_usb_port *port, struct typec_mux_state *state)
 	struct altmode_req req = { };
 	u8 cable_speed;
 
+	if (IOM_PORT_ACTIVITY_IS(port->iom_status, TBT) ||
+	    IOM_PORT_ACTIVITY_IS(port->iom_status, ALT_MODE_TBT_USB))
+		return 0;
+
 	req.usage = PMC_USB_ALT_MODE;
 	req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 	req.mode_type = PMC_USB_MODE_TYPE_TBT << PMC_USB_MODE_TYPE_SHIFT;
@@ -273,6 +303,9 @@ static int pmc_usb_mux_safe_state(struct pmc_usb_port *port)
 {
 	u8 msg;
 
+	if (IOM_PORT_ACTIVITY_IS(port->iom_status, SAFE_MODE))
+		return 0;
+
 	msg = PMC_USB_SAFE_MODE;
 	msg |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 
@@ -283,6 +316,9 @@ static int pmc_usb_connect(struct pmc_usb_port *port)
 {
 	u8 msg[2];
 
+	if (port->iom_status & IOM_PORT_STATUS_CONNECTED)
+		return 0;
+
 	msg[0] = PMC_USB_CONNECT;
 	msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 
@@ -295,8 +331,16 @@ static int pmc_usb_connect(struct pmc_usb_port *port)
 
 static int pmc_usb_disconnect(struct pmc_usb_port *port)
 {
+	struct typec_displayport_data data = { };
 	u8 msg[2];
 
+	if (!(port->iom_status & IOM_PORT_STATUS_CONNECTED))
+		return 0;
+
+	/* Clear DisplayPort HPD if it's still asserted. */
+	if (IOM_PORT_HPD_ASSERTED(port->iom_status))
+		pmc_usb_mux_dp_hpd(port, &data);
+
 	msg[0] = PMC_USB_DISCONNECT;
 	msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 
@@ -309,6 +353,11 @@ static int
 pmc_usb_mux_set(struct typec_mux *mux, struct typec_mux_state *state)
 {
 	struct pmc_usb_port *port = typec_mux_get_drvdata(mux);
+	int ret;
+
+	ret = intel_iom_port_status(port->num, &port->iom_status);
+	if (ret)
+		return ret;
 
 	if (port->orientation == TYPEC_ORIENTATION_NONE || port->role == USB_ROLE_NONE)
 		return 0;
@@ -344,9 +393,11 @@ static int pmc_usb_set_orientation(struct typec_switch *sw,
 				   enum typec_orientation orientation)
 {
 	struct pmc_usb_port *port = typec_switch_get_drvdata(sw);
+	int ret;
 
-	if (port->orientation == orientation)
-		return 0;
+	ret = intel_iom_port_status(port->num, &port->iom_status);
+	if (ret)
+		return ret;
 
 	port->orientation = orientation;
 
@@ -363,9 +414,11 @@ static int pmc_usb_set_orientation(struct typec_switch *sw,
 static int pmc_usb_set_role(struct usb_role_switch *sw, enum usb_role role)
 {
 	struct pmc_usb_port *port = usb_role_switch_get_drvdata(sw);
+	int ret;
 
-	if (port->role == role)
-		return 0;
+	ret = intel_iom_port_status(port->num, &port->iom_status);
+	if (ret)
+		return ret;
 
 	port->role = role;
 
-- 
2.20.1

[PATCH v2 3/3] usb: typec: intel_pmc_mux: Support for device role (UFP)

[Rajmohan Mani]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This adds support for device data role, and data role
swapping. The driver no longer relies on the cached role, as
it may not be valid (for example after bootup). Instead, the
role is always checked by readding the port status from IOM.

Note. After this, the orientation is always only cached, so
the driver does not support scenario where the role is set
before orientation. It means the typec drivers must always
set the orientation first before role.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/usb/typec/mux/intel_pmc_mux.c | 67 ++++++++++++++-------------
 1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index c758d6113c41..d4ee7132227b 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -191,9 +191,6 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
 		return pmc_usb_mux_dp_hpd(port, state->data);
 	}
 
-	if (data->status & DP_STATUS_IRQ_HPD)
-		return pmc_usb_mux_dp_hpd(port, state->data);
-
 	req.usage = PMC_USB_ALT_MODE;
 	req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 	req.mode_type = PMC_USB_MODE_TYPE_DP << PMC_USB_MODE_TYPE_SHIFT;
@@ -312,39 +309,52 @@ static int pmc_usb_mux_safe_state(struct pmc_usb_port *port)
 	return pmc_usb_command(port, &msg, sizeof(msg));
 }
 
-static int pmc_usb_connect(struct pmc_usb_port *port)
+static int pmc_usb_disconnect(struct pmc_usb_port *port)
 {
+	struct typec_displayport_data data = { };
 	u8 msg[2];
 
-	if (port->iom_status & IOM_PORT_STATUS_CONNECTED)
+	if (!(port->iom_status & IOM_PORT_STATUS_CONNECTED))
 		return 0;
 
-	msg[0] = PMC_USB_CONNECT;
+	/* Clear DisplayPort HPD if it's still asserted. */
+	if (IOM_PORT_HPD_ASSERTED(port->iom_status))
+		pmc_usb_mux_dp_hpd(port, &data);
+
+	msg[0] = PMC_USB_DISCONNECT;
 	msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 
 	msg[1] = port->usb2_port << PMC_USB_MSG_USB2_PORT_SHIFT;
-	msg[1] |= hsl_orientation(port) << PMC_USB_MSG_ORI_HSL_SHIFT;
-	msg[1] |= sbu_orientation(port) << PMC_USB_MSG_ORI_AUX_SHIFT;
 
 	return pmc_usb_command(port, msg, sizeof(msg));
 }
 
-static int pmc_usb_disconnect(struct pmc_usb_port *port)
+static int pmc_usb_connect(struct pmc_usb_port *port, enum usb_role role)
 {
-	struct typec_displayport_data data = { };
+	u8 ufp = role == USB_ROLE_DEVICE ? 1 : 0;
 	u8 msg[2];
+	int ret;
 
-	if (!(port->iom_status & IOM_PORT_STATUS_CONNECTED))
-		return 0;
+	if (port->orientation == TYPEC_ORIENTATION_NONE)
+		return -EINVAL;
 
-	/* Clear DisplayPort HPD if it's still asserted. */
-	if (IOM_PORT_HPD_ASSERTED(port->iom_status))
-		pmc_usb_mux_dp_hpd(port, &data);
+	if (port->iom_status & IOM_PORT_STATUS_CONNECTED) {
+		if (port->role == role || port->role == USB_ROLE_NONE)
+			return 0;
 
-	msg[0] = PMC_USB_DISCONNECT;
+		/* Role swap */
+		ret = pmc_usb_disconnect(port);
+		if (ret)
+			return ret;
+	}
+
+	msg[0] = PMC_USB_CONNECT;
 	msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
 
 	msg[1] = port->usb2_port << PMC_USB_MSG_USB2_PORT_SHIFT;
+	msg[1] |= ufp << PMC_USB_MSG_UFP_SHIFT;
+	msg[1] |= hsl_orientation(port) << PMC_USB_MSG_ORI_HSL_SHIFT;
+	msg[1] |= sbu_orientation(port) << PMC_USB_MSG_ORI_AUX_SHIFT;
 
 	return pmc_usb_command(port, msg, sizeof(msg));
 }
@@ -365,7 +375,7 @@ pmc_usb_mux_set(struct typec_mux *mux, struct typec_mux_state *state)
 	if (state->mode == TYPEC_STATE_SAFE)
 		return pmc_usb_mux_safe_state(port);
 	if (state->mode == TYPEC_STATE_USB)
-		return pmc_usb_connect(port);
+		return pmc_usb_connect(port, port->role);
 
 	if (state->alt) {
 		switch (state->alt->svid) {
@@ -380,7 +390,7 @@ pmc_usb_mux_set(struct typec_mux *mux, struct typec_mux_state *state)
 			/* REVISIT: Try with usb3_port set to 0? */
 			break;
 		case TYPEC_MODE_USB3:
-			return pmc_usb_connect(port);
+			return pmc_usb_connect(port, port->role);
 		case TYPEC_MODE_USB4:
 			return pmc_usb_mux_usb4(port, state);
 		}
@@ -401,13 +411,6 @@ static int pmc_usb_set_orientation(struct typec_switch *sw,
 
 	port->orientation = orientation;
 
-	if (port->role) {
-		if (orientation == TYPEC_ORIENTATION_NONE)
-			return pmc_usb_disconnect(port);
-		else
-			return pmc_usb_connect(port);
-	}
-
 	return 0;
 }
 
@@ -420,16 +423,14 @@ static int pmc_usb_set_role(struct usb_role_switch *sw, enum usb_role role)
 	if (ret)
 		return ret;
 
-	port->role = role;
+	if (role == USB_ROLE_NONE)
+		ret = pmc_usb_disconnect(port);
+	else
+		ret = pmc_usb_connect(port, role);
 
-	if (port->orientation) {
-		if (role == USB_ROLE_NONE)
-			return pmc_usb_disconnect(port);
-		else
-			return pmc_usb_connect(port);
-	}
+	port->role = role;
 
-	return 0;
+	return ret;
 }
 
 static int pmc_usb_register_port(struct pmc_usb *pmc, int index,
-- 
2.20.1

Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Prashant Malani]

Hi Rajmohan,

On Fri, Aug 21, 2020 at 09:05:06PM -0700, Rajmohan Mani wrote:
> Input Output Manager (IOM) is part of the Tiger Lake SoC that
> configures the Type-C Sub System (TCSS). IOM is a micro controller
> that handles Type-C topology, configuration and PM functions of
> various Type-C devices connected on the platform.
> 
> This driver helps read relevant information such as Type-C port
> status (whether a device is connected to a Type-C port or not) and
> the activity type on the Type-C ports (such as USB, Display Port,
> Thunderbolt), for consumption by other drivers.
> 
> Currently intel_iom_port_status() API is exported by this driver,
> that has information about the Type-C port status and port activity
> type.
> 
> Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> ---

Perhaps include a version log of changes since v1?
> diff --git a/drivers/platform/x86/intel_iom.c b/drivers/platform/x86/intel_iom.c
> new file mode 100644
> index 000000000000..cda7716410c6
> --- /dev/null
> +++ b/drivers/platform/x86/intel_iom.c
> +int intel_iom_port_status(u8 port, u32 *status)
> +{
> +	void __iomem *reg;
> +
> +	if (!iom || !iom->dev || !iom->regbar)

Do we need to check for !iom->dev and !iom->regbar? Is there a valid
situation where iom != NULL but iom->dev and/or iom->regbar == NULL?
Sounds like it shouldn't, but I may be missing something.

> +		return -ENODEV;
> +
> +	if (!status || (port > IOM_MAX_PORTS - 1))

I think parentheses around "port > IOM_MAX_PORT - 1" aren't required.
> +		return -EINVAL;
> +
> +	reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN * port;
> +
> +	*status = ioread32(reg);

Perhaps just inline reg within the parentheses?
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_iom_port_status);
> +
> +static int intel_iom_probe(struct platform_device *pdev)
> +{
> +	void __iomem *addr;
> +
> +	/* only one IOM device is supported */

Minor nit: s/only/Only
> +	if (iom)
> +		return -EBUSY;
> +
> +	iom = devm_kzalloc(&pdev->dev, sizeof(*iom), GFP_KERNEL);
> +	if (!iom)
> +		return -ENOMEM;
> +
> +	addr = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(addr))
> +		return PTR_ERR(addr);
> +
> +	iom->regbar = addr;
> +	iom->dev = &pdev->dev;
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id intel_iom_acpi_ids[] = {
> +	{ "INTC1072" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, intel_iom_acpi_ids);
> +
> +static struct platform_driver intel_iom_driver = {
> +	.probe = intel_iom_probe,

nit: I generally see ".probe" listed below ".driver".
> +	.driver = {
> +		.name = "intel_iom",
> +		.acpi_match_table = intel_iom_acpi_ids,
> +	},
> +};
> +
> +module_platform_driver_probe(intel_iom_driver, intel_iom_probe);
> +
> +MODULE_AUTHOR("Rajmohan Mani <rajmohan.mani@intel.com>");
> +MODULE_DESCRIPTION("Intel IOM driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/x86/intel_iom.h b/include/linux/platform_data/x86/intel_iom.h
> new file mode 100644
> index 000000000000..e4c9a305e7a9
> --- /dev/null
> +++ b/include/linux/platform_data/x86/intel_iom.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _PLATFORM_DATA_X86_INTEL_IOM_H_
> +#define _PLATFORM_DATA_X86_INTEL_IOM_H_
> +
> +
> +#define IOM_MAX_PORTS					4
> +/* Register length in bytes */
> +#define IOM_REG_LEN					4

Do these two #define's need to be in the header, instead of directly in
intel_iom.c ?

> +
> +#ifdef CONFIG_ACPI
> +
> +int intel_iom_port_status(u8 port, u32 *status);
> +
> +#else
> +
> +int intel_iom_port_status(struct intel_iom *iom, u8 port, u32 *status)

Should the function signature be the same as the #ifdef case?

Best regards,

-Prashant

RE: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

Hi Prashant,

Thanks for the quick review.

> Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager
> (IOM) driver
> 
> Hi Rajmohan,
> 
> On Fri, Aug 21, 2020 at 09:05:06PM -0700, Rajmohan Mani wrote:
> > Input Output Manager (IOM) is part of the Tiger Lake SoC that
> > configures the Type-C Sub System (TCSS). IOM is a micro controller
> > that handles Type-C topology, configuration and PM functions of
> > various Type-C devices connected on the platform.
> >
> > This driver helps read relevant information such as Type-C port status
> > (whether a device is connected to a Type-C port or not) and the
> > activity type on the Type-C ports (such as USB, Display Port,
> > Thunderbolt), for consumption by other drivers.
> >
> > Currently intel_iom_port_status() API is exported by this driver, that
> > has information about the Type-C port status and port activity type.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> > ---
> 
> Perhaps include a version log of changes since v1?

Yes. It's there in the cover letter (patch v2 0/3).

> > diff --git a/drivers/platform/x86/intel_iom.c
> > b/drivers/platform/x86/intel_iom.c
> > new file mode 100644
> > index 000000000000..cda7716410c6
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_iom.c
> > +int intel_iom_port_status(u8 port, u32 *status) {
> > +	void __iomem *reg;
> > +
> > +	if (!iom || !iom->dev || !iom->regbar)
> 
> Do we need to check for !iom->dev and !iom->regbar?

It's a good practice to have sanity checks on pointer members dereferenced.

So I can lose the check on iom->dev, but prefer to keep the check on regbar.
Let me know if you feel strongly about losing the check for regbar as well.

> Is there a valid situation
> where iom != NULL but iom->dev and/or iom->regbar == NULL?
> Sounds like it shouldn't, but I may be missing something.
> 

I think I am being conservative here.

> > +		return -ENODEV;
> > +
> > +	if (!status || (port > IOM_MAX_PORTS - 1))
> 
> I think parentheses around "port > IOM_MAX_PORT - 1" aren't required.

Ack

> > +		return -EINVAL;
> > +
> > +	reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN *
> port;
> > +
> > +	*status = ioread32(reg);
> 
> Perhaps just inline reg within the parentheses?

Kept this way to increase readability. Let me know if you feel strongly towards
inline reg.

> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(intel_iom_port_status);
> > +
> > +static int intel_iom_probe(struct platform_device *pdev) {
> > +	void __iomem *addr;
> > +
> > +	/* only one IOM device is supported */
> 
> Minor nit: s/only/Only

And then I may need to end the comment with a period.
Let me know if you feel strongly.

> > +	if (iom)
> > +		return -EBUSY;
> > +
> > +	iom = devm_kzalloc(&pdev->dev, sizeof(*iom), GFP_KERNEL);
> > +	if (!iom)
> > +		return -ENOMEM;
> > +
> > +	addr = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(addr))
> > +		return PTR_ERR(addr);
> > +
> > +	iom->regbar = addr;
> > +	iom->dev = &pdev->dev;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct acpi_device_id intel_iom_acpi_ids[] = {
> > +	{ "INTC1072" },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(acpi, intel_iom_acpi_ids);
> > +
> > +static struct platform_driver intel_iom_driver = {
> > +	.probe = intel_iom_probe,
> 
> nit: I generally see ".probe" listed below ".driver".

Ack

> > +	.driver = {
> > +		.name = "intel_iom",
> > +		.acpi_match_table = intel_iom_acpi_ids,
> > +	},
> > +};
> > +
> > +module_platform_driver_probe(intel_iom_driver, intel_iom_probe);
> > +
> > +MODULE_AUTHOR("Rajmohan Mani <rajmohan.mani@intel.com>");
> > +MODULE_DESCRIPTION("Intel IOM driver"); MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/platform_data/x86/intel_iom.h
> > b/include/linux/platform_data/x86/intel_iom.h
> > new file mode 100644
> > index 000000000000..e4c9a305e7a9
> > --- /dev/null
> > +++ b/include/linux/platform_data/x86/intel_iom.h
> > @@ -0,0 +1,49 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _PLATFORM_DATA_X86_INTEL_IOM_H_ #define
> > +_PLATFORM_DATA_X86_INTEL_IOM_H_
> > +
> > +
> > +#define IOM_MAX_PORTS					4
> > +/* Register length in bytes */
> > +#define IOM_REG_LEN					4
> 
> Do these two #define's need to be in the header, instead of directly in
> intel_iom.c ?
> 

Ack. These 2 can be moved to .c file.

> > +
> > +#ifdef CONFIG_ACPI
> > +
> > +int intel_iom_port_status(u8 port, u32 *status);
> > +
> > +#else
> > +
> > +int intel_iom_port_status(struct intel_iom *iom, u8 port, u32
> > +*status)
> 
> Should the function signature be the same as the #ifdef case?
> 

Thanks for catching this. I missed it.

Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Prashant Malani]

Hi Rajmohan,

On Mon, Aug 24, 2020 at 10:19:27PM +0000, Mani, Rajmohan wrote:
> Hi Prashant,
> 
> Thanks for the quick review.
> 
> > > +
> > > +	if (!iom || !iom->dev || !iom->regbar)
> > 
> > Do we need to check for !iom->dev and !iom->regbar?
> 
> It's a good practice to have sanity checks on pointer members dereferenced.
> 
> So I can lose the check on iom->dev, but prefer to keep the check on regbar.
> Let me know if you feel strongly about losing the check for regbar as well.

Sounds good.
> 
> > Is there a valid situation
> > where iom != NULL but iom->dev and/or iom->regbar == NULL?
> > Sounds like it shouldn't, but I may be missing something.
> > 
> 
> I think I am being conservative here.
> 
> > > +		return -ENODEV;
> > > +
> > > +	if (!status || (port > IOM_MAX_PORTS - 1))
> > 
> > I think parentheses around "port > IOM_MAX_PORT - 1" aren't required.
> 
> Ack
> 
> > > +		return -EINVAL;
> > > +
> > > +	reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN *
> > port;
> > > +
> > > +	*status = ioread32(reg);
> > 
> > Perhaps just inline reg within the parentheses?
> 
> Kept this way to increase readability. Let me know if you feel strongly towards
> inline reg.

I'd rather this be inlined, you save a couple lines from the variable
declaration, and IMO we're not gaining much in terms of readability by
declaring this separately.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(intel_iom_port_status);
> > > +
> > > +static int intel_iom_probe(struct platform_device *pdev) {
> > > +	void __iomem *addr;
> > > +
> > > +	/* only one IOM device is supported */
> > 
> > Minor nit: s/only/Only
> 
> And then I may need to end the comment with a period.
> Let me know if you feel strongly.
Yes, let's capitalize and add the period. Let's try to use the right
punctuation where possible.

Best regards,

-Prashant

RE: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

Hi Prashant,

...

> > > > +
> > > > +	reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN *
> > > port;
> > > > +
> > > > +	*status = ioread32(reg);
> > >
> > > Perhaps just inline reg within the parentheses?
> >
> > Kept this way to increase readability. Let me know if you feel
> > strongly towards inline reg.
> 
> I'd rather this be inlined, you save a couple lines from the variable declaration,
> and IMO we're not gaining much in terms of readability by declaring this
> separately.
> 

Ack
(at least to me, it was more readable)

...

> >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_iom_port_status);
> > > > +
> > > > +static int intel_iom_probe(struct platform_device *pdev) {
> > > > +	void __iomem *addr;
> > > > +
> > > > +	/* only one IOM device is supported */
> > >
> > > Minor nit: s/only/Only
> >
> > And then I may need to end the comment with a period.
> > Let me know if you feel strongly.
> Yes, let's capitalize and add the period. Let's try to use the right punctuation
> where possible.
> 

Ack
Will take care of this as part of v3.

Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Greg Kroah-Hartman]

On Fri, Aug 21, 2020 at 09:05:06PM -0700, Rajmohan Mani wrote:
> Input Output Manager (IOM) is part of the Tiger Lake SoC that
> configures the Type-C Sub System (TCSS). IOM is a micro controller
> that handles Type-C topology, configuration and PM functions of
> various Type-C devices connected on the platform.
> 
> This driver helps read relevant information such as Type-C port
> status (whether a device is connected to a Type-C port or not) and
> the activity type on the Type-C ports (such as USB, Display Port,
> Thunderbolt), for consumption by other drivers.
> 
> Currently intel_iom_port_status() API is exported by this driver,
> that has information about the Type-C port status and port activity
> type.
> 
> Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
> ---
>  drivers/platform/x86/Kconfig                | 16 ++++
>  drivers/platform/x86/Makefile               |  1 +
>  drivers/platform/x86/intel_iom.c            | 95 +++++++++++++++++++++
>  include/linux/platform_data/x86/intel_iom.h | 49 +++++++++++
>  4 files changed, 161 insertions(+)
>  create mode 100644 drivers/platform/x86/intel_iom.c
>  create mode 100644 include/linux/platform_data/x86/intel_iom.h
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 40219bba6801..f09b4b525ea5 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -837,6 +837,22 @@ config INTEL_INT0002_VGPIO
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called intel_int0002_vgpio.
>  
> +config INTEL_IOM
> +	tristate "Intel Input Output Manager (IOM) driver"
> +	depends on ACPI && PCI
> +	help
> +	  This driver helps read relevant information such as Type-C port
> +	  status (whether a device is connected to a Type-C port or not)
> +	  and the activity type on the Type-C ports (such as USB, Display
> +	  Port, Thunderbolt), for consumption by other drivers.
> +
> +	  Currently intel_iom_port_status() API is exported by this driver,
> +	  that has information about the Type-C port status and port activity
> +	  type.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called intel_iom.
> +
>  config INTEL_MENLOW
>  	tristate "Thermal Management driver for Intel menlow platform"
>  	depends on ACPI_THERMAL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 5f823f7eff45..b44f706074c3 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -77,6 +77,7 @@ intel_cht_int33fe-objs			:= intel_cht_int33fe_common.o \
>  					   intel_cht_int33fe_microb.o
>  obj-$(CONFIG_INTEL_HID_EVENT)		+= intel-hid.o
>  obj-$(CONFIG_INTEL_INT0002_VGPIO)	+= intel_int0002_vgpio.o
> +obj-$(CONFIG_INTEL_IOM)			+= intel_iom.o
>  obj-$(CONFIG_INTEL_MENLOW)		+= intel_menlow.o
>  obj-$(CONFIG_INTEL_OAKTRAIL)		+= intel_oaktrail.o
>  obj-$(CONFIG_INTEL_VBTN)		+= intel-vbtn.o
> diff --git a/drivers/platform/x86/intel_iom.c b/drivers/platform/x86/intel_iom.c
> new file mode 100644
> index 000000000000..cda7716410c6
> --- /dev/null
> +++ b/drivers/platform/x86/intel_iom.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Core SoC Input Output Manager (IOM) driver.
> + *
> + * This driver provides access to the Input Output Manager (IOM) (that
> + * is part of Tiger Lake SoC) registers that can be used to know about
> + * Type-C Sub System related information (such as Type-C port status,
> + * activity type on Type-C ports).
> + *
> + * Copyright (C) 2020, Intel Corporation
> + * Author: Rajmohan Mani <rajmohan.mani@intel.com>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/x86/intel_iom.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define IOM_PORT_STATUS_OFFSET				0x560
> +
> +struct intel_iom {
> +	struct device *dev;
> +	void __iomem *regbar;
> +};
> +
> +static struct intel_iom *iom;
> +
> +/**
> + * intel_iom_port_status() - Get status bits for the Type-C port
> + * @port: Type-C port number
> + * @status: pointer to receive the status bits
> + *
> + * Returns 0 on success, error otherwise.
> + */
> +int intel_iom_port_status(u8 port, u32 *status)
> +{
> +	void __iomem *reg;
> +
> +	if (!iom || !iom->dev || !iom->regbar)
> +		return -ENODEV;
> +
> +	if (!status || (port > IOM_MAX_PORTS - 1))
> +		return -EINVAL;
> +
> +	reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN * port;
> +
> +	*status = ioread32(reg);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_iom_port_status);
> +
> +static int intel_iom_probe(struct platform_device *pdev)
> +{
> +	void __iomem *addr;
> +
> +	/* only one IOM device is supported */
> +	if (iom)
> +		return -EBUSY;
> +
> +	iom = devm_kzalloc(&pdev->dev, sizeof(*iom), GFP_KERNEL);
> +	if (!iom)
> +		return -ENOMEM;
> +
> +	addr = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(addr))
> +		return PTR_ERR(addr);
> +
> +	iom->regbar = addr;
> +	iom->dev = &pdev->dev;
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id intel_iom_acpi_ids[] = {
> +	{ "INTC1072" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, intel_iom_acpi_ids);
> +
> +static struct platform_driver intel_iom_driver = {
> +	.probe = intel_iom_probe,
> +	.driver = {
> +		.name = "intel_iom",
> +		.acpi_match_table = intel_iom_acpi_ids,
> +	},
> +};
> +
> +module_platform_driver_probe(intel_iom_driver, intel_iom_probe);
> +
> +MODULE_AUTHOR("Rajmohan Mani <rajmohan.mani@intel.com>");
> +MODULE_DESCRIPTION("Intel IOM driver");
> +MODULE_LICENSE("GPL v2");

I still find this crazy that a whole separate driver is created just to
read a single 32bit value.

Why not put this logic in the driver that wants to read that value?
That would be much simpler, smaller, and more obvious.

thanks,

greg k-h

Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Heikki Krogerus]

Hi Greg,

On Fri, Aug 28, 2020 at 09:43:59AM +0200, Greg Kroah-Hartman wrote:
> I still find this crazy that a whole separate driver is created just to
> read a single 32bit value.
> 
> Why not put this logic in the driver that wants to read that value?
> That would be much simpler, smaller, and more obvious.

That would mean that we start maintaining something like DMI quirk
table in those drivers. Unfortunately the IOM device is not available
on every platform. Also, even on platforms that do have it, there is
no guarantee that the device is always going to be mapped to the same
address.

Nevertheless, I was originally hoping that we could hide the handling
of IOM somehow in ACPI without the need for an actual device object,
but it now turns out that the other features of the IOM chip have
created interest. At least our i915 guys probable have some use for it
(I don't know exactly what they are planning to use it for).

So the fact that we may later need the device for something else, on
top of the clumsiness and most importantly risks involved with using
ACPI to take care of extra tasks (ASL tends to have bugs - bugs that
may never ever get fixed), I think the IOM device object, and the
driver that binds to it, do have a valid reason for existing.


thanks,

-- 
heikki

Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Greg Kroah-Hartman]

On Fri, Aug 28, 2020 at 12:08:32PM +0300, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Fri, Aug 28, 2020 at 09:43:59AM +0200, Greg Kroah-Hartman wrote:
> > I still find this crazy that a whole separate driver is created just to
> > read a single 32bit value.
> > 
> > Why not put this logic in the driver that wants to read that value?
> > That would be much simpler, smaller, and more obvious.
> 
> That would mean that we start maintaining something like DMI quirk
> table in those drivers. Unfortunately the IOM device is not available
> on every platform. Also, even on platforms that do have it, there is
> no guarantee that the device is always going to be mapped to the same
> address.

Ok, but putting it in another driver has the same problem as you are now
depending on that driver with the driver that calls this, right?

The quirk table would go in here, but not somewhere else?

And if this really is only for typec stuff, put it in the typec
directory, not burried off in the platform driver subdir please.

> Nevertheless, I was originally hoping that we could hide the handling
> of IOM somehow in ACPI without the need for an actual device object,
> but it now turns out that the other features of the IOM chip have
> created interest. At least our i915 guys probable have some use for it
> (I don't know exactly what they are planning to use it for).
> 
> So the fact that we may later need the device for something else, on
> top of the clumsiness and most importantly risks involved with using
> ACPI to take care of extra tasks (ASL tends to have bugs - bugs that
> may never ever get fixed), I think the IOM device object, and the
> driver that binds to it, do have a valid reason for existing.

Handle the situation today, if, in the future, someone else needs/wants
this, _then_ work on splitting it out into separate pieces.  Don't
create additional complexity today, for no benefit today.  It's already
caused numerous review comments/complaints the way this is designed...

thanks,

greg k-h

Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Heikki Krogerus]

On Fri, Aug 28, 2020 at 12:03:35PM +0200, Greg Kroah-Hartman wrote:
> Handle the situation today, if, in the future, someone else needs/wants
> this, _then_ work on splitting it out into separate pieces.  Don't
> create additional complexity today, for no benefit today.  It's already
> caused numerous review comments/complaints the way this is designed...

OK. We'll handle this in the mux driver for now.


thanks,

-- 
heikki

RE: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

Hi Greg,

> Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager
> (IOM) driver
> 
> Hi Greg,
> 
> On Fri, Aug 28, 2020 at 09:43:59AM +0200, Greg Kroah-Hartman wrote:
> > I still find this crazy that a whole separate driver is created just
> > to read a single 32bit value.
> >
> > Why not put this logic in the driver that wants to read that value?
> > That would be much simpler, smaller, and more obvious.
> 
> That would mean that we start maintaining something like DMI quirk table in
> those drivers. Unfortunately the IOM device is not available on every platform.
> Also, even on platforms that do have it, there is no guarantee that the device is
> always going to be mapped to the same address.
> 
> Nevertheless, I was originally hoping that we could hide the handling of IOM
> somehow in ACPI without the need for an actual device object, but it now
> turns out that the other features of the IOM chip have created interest. At
> least our i915 guys probable have some use for it (I don't know exactly what
> they are planning to use it for).
> 
> So the fact that we may later need the device for something else, on top of the
> clumsiness and most importantly risks involved with using ACPI to take care of
> extra tasks (ASL tends to have bugs - bugs that may never ever get fixed), I
> think the IOM device object, and the driver that binds to it, do have a valid
> reason for existing.
> 

Intel PMC USB mux device is part of the PCH, while IOM is part of the SoC.
This was another reason we had to have a separate ACPI device.

> 
> thanks,
> 
> --
> heikki

RE: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

> Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager
> (IOM) driver
> 
> On Fri, Aug 28, 2020 at 12:03:35PM +0200, Greg Kroah-Hartman wrote:
> > Handle the situation today, if, in the future, someone else
> > needs/wants this, _then_ work on splitting it out into separate
> > pieces.  Don't create additional complexity today, for no benefit
> > today.  It's already caused numerous review comments/complaints the way
> this is designed...
> 
> OK. We'll handle this in the mux driver for now.
> 

Ack. Will work with Heikki on this.

> 
> thanks,
> 
> --
> heikki

Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Greg Kroah-Hartman]

On Fri, Aug 28, 2020 at 03:20:22PM +0000, Mani, Rajmohan wrote:
> Hi Greg,
> 
> > Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager
> > (IOM) driver
> > 
> > Hi Greg,
> > 
> > On Fri, Aug 28, 2020 at 09:43:59AM +0200, Greg Kroah-Hartman wrote:
> > > I still find this crazy that a whole separate driver is created just
> > > to read a single 32bit value.
> > >
> > > Why not put this logic in the driver that wants to read that value?
> > > That would be much simpler, smaller, and more obvious.
> > 
> > That would mean that we start maintaining something like DMI quirk table in
> > those drivers. Unfortunately the IOM device is not available on every platform.
> > Also, even on platforms that do have it, there is no guarantee that the device is
> > always going to be mapped to the same address.
> > 
> > Nevertheless, I was originally hoping that we could hide the handling of IOM
> > somehow in ACPI without the need for an actual device object, but it now
> > turns out that the other features of the IOM chip have created interest. At
> > least our i915 guys probable have some use for it (I don't know exactly what
> > they are planning to use it for).
> > 
> > So the fact that we may later need the device for something else, on top of the
> > clumsiness and most importantly risks involved with using ACPI to take care of
> > extra tasks (ASL tends to have bugs - bugs that may never ever get fixed), I
> > think the IOM device object, and the driver that binds to it, do have a valid
> > reason for existing.
> > 
> 
> Intel PMC USB mux device is part of the PCH, while IOM is part of the SoC.

I have no idea what a "PCH" is, what "IOM" is, and how any of this
relates to a "SoC" :)

Don't impose arbritrary hardware "splits" to kernel code when the kernel
has no such "partitioning" please.

> This was another reason we had to have a separate ACPI device.

That sounds like a firmware issue you can solve in UEFI.

I think this is the most TLA-laden email I have ever written, and I used
to work at IBM :)

greg k-h

RE: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

Hi Greg,

> Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager
> (IOM) driver
> 
> On Fri, Aug 28, 2020 at 03:20:22PM +0000, Mani, Rajmohan wrote:
> > Hi Greg,
> >
> > > Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output
> > > Manager
> > > (IOM) driver
> > >
> > > Hi Greg,
> > >
> > > On Fri, Aug 28, 2020 at 09:43:59AM +0200, Greg Kroah-Hartman wrote:
> > > > I still find this crazy that a whole separate driver is created
> > > > just to read a single 32bit value.
> > > >
> > > > Why not put this logic in the driver that wants to read that value?
> > > > That would be much simpler, smaller, and more obvious.
> > >
> > > That would mean that we start maintaining something like DMI quirk
> > > table in those drivers. Unfortunately the IOM device is not available on
> every platform.
> > > Also, even on platforms that do have it, there is no guarantee that
> > > the device is always going to be mapped to the same address.
> > >
> > > Nevertheless, I was originally hoping that we could hide the
> > > handling of IOM somehow in ACPI without the need for an actual
> > > device object, but it now turns out that the other features of the
> > > IOM chip have created interest. At least our i915 guys probable have
> > > some use for it (I don't know exactly what they are planning to use it for).
> > >
> > > So the fact that we may later need the device for something else, on
> > > top of the clumsiness and most importantly risks involved with using
> > > ACPI to take care of extra tasks (ASL tends to have bugs - bugs that
> > > may never ever get fixed), I think the IOM device object, and the
> > > driver that binds to it, do have a valid reason for existing.
> > >
> >
> > Intel PMC USB mux device is part of the PCH, while IOM is part of the SoC.
> 
> I have no idea what a "PCH" is, what "IOM" is, and how any of this relates to a
> "SoC" :)
> 

I was just meaning to say IOM (Intel Output Manager) is a separate device, that
is not part of PCH (Platform Controller Hub) like PMC (Power Management Controller).

For the sake of completeness

PCH - Platform Controller Hub (usually that handles I/Os in Intel core platforms)
IOM - Input Output Manager (IOM) is part of the Tiger Lake SoC that handles Type-C
topology, configuration and PM functions of various Type-C devices connected
on the platform

> Don't impose arbritrary hardware "splits" to kernel code when the kernel has
> no such "partitioning" please.
> 

Ack.

> > This was another reason we had to have a separate ACPI device.
> 
> That sounds like a firmware issue you can solve in UEFI.
> 

Ack

> I think this is the most TLA-laden email I have ever written, and I used to work
> at IBM :)

I thought it was only Intel where TLAs are abundantly used.

Thanks for the reviews and the direction on this topic.

> 
> greg k-h