All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V4 0/7] Add USB3.0 and TI HD3SS3220 driver support
@ 2019-04-16  9:38 Biju Das
  2019-04-16  9:38   ` [V4,1/7] " Biju Das
                   ` (6 more replies)
  0 siblings, 7 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Heikki Krogerus, Felipe Balbi,
	Mark Rutland
  Cc: Biju Das, linux-usb, devicetree, Simon Horman, Yoshihiro Shimoda,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	linux-renesas-soc

This series adds USB 3.0 support for the CAT874 platform, including a
new driver for the TI HD3SS3220 USB Type-C DRP port controller.

This patch series supports:
1) Host hotplug operation
2) Device hot plug operation
3) USB type-C data_role switch
   (Tested with 2 RZ/G2E boards connected with a Type-C cable)

This patchset is based on linux_next next-20190411 branch.
V3-->V4
  * Incorporated Chunfeng Yun's review comment
    (https://patchwork.kernel.org/project/linux-usb/list/?submitter=133171)
  * Used fwnode API's to get roleswitch handle

V2-->V3
  * Used the new API to usb_role_switch by node to find the remote endpoint
    (https://patchwork.kernel.org/patch/10882555/)
  * Added renesas,usb-role-switch property
  * Incorporated shimoda-san's review comment
    (https://patchwork.kernel.org/patch/10852507/)

V1-->V2
  * Use USB role class instead of extcon to receive connect and disconnect
    events and also for the dual role switch.
  * Dropped patch 6
  * Squashed patch 8 and patch 9
  * https://patchwork.kernel.org/cover/10840641/


Biju Das (7):
  dt-bindings: usb: hd3ss3220 device tree binding document
  dt-bindings: usb: renesas_usb3: Add renesas,usb-role-switch property
  usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller
  usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
  arm64: defconfig: enable TYPEC_HD3SS3220 config option
  arm64: dts: renesas: r8a774c0-cat874: Enable USB3.0 host/peripheral
    device node
  arm64: dts: renesas: r8a774c0-cat874: Enable usb role switch support

 .../devicetree/bindings/usb/renesas_usb3.txt       |  22 ++
 .../devicetree/bindings/usb/ti,hd3ss3220.txt       |  37 +++
 arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts    |  56 +++++
 arch/arm64/configs/defconfig                       |   2 +
 drivers/usb/gadget/udc/renesas_usb3.c              | 126 ++++++++--
 drivers/usb/typec/Kconfig                          |  10 +
 drivers/usb/typec/Makefile                         |   1 +
 drivers/usb/typec/hd3ss3220.c                      | 263 +++++++++++++++++++++
 8 files changed, 495 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/ti,hd3ss3220.txt
 create mode 100644 drivers/usb/typec/hd3ss3220.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH V4 1/7] dt-bindings: usb: hd3ss3220 device tree binding document
@ 2019-04-16  9:38   ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: Biju Das, Greg Kroah-Hartman, Heikki Krogerus, Felipe Balbi,
	linux-usb, devicetree, Simon Horman, Yoshihiro Shimoda,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	linux-renesas-soc

Add device tree binding document for TI HD3SS3220 Type-C DRP port
controller driver.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
V3-->V4
  * No Change.
V2-->V3
  * Added Rob's Reviewed by tag.
V1-->V2
  * Added connector node.
  * updated the example with connector node.
---
 .../devicetree/bindings/usb/ti,hd3ss3220.txt       | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/ti,hd3ss3220.txt

diff --git a/Documentation/devicetree/bindings/usb/ti,hd3ss3220.txt b/Documentation/devicetree/bindings/usb/ti,hd3ss3220.txt
new file mode 100644
index 0000000..7f41400
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/ti,hd3ss3220.txt
@@ -0,0 +1,37 @@
+TI HD3SS3220 TypeC DRP Port Controller.
+
+Required properties:
+ - compatible: Must be "ti,hd3ss3220".
+ - reg: I2C slave address, must be 0x47 or 0x67 based on ADDR pin.
+ - interrupts: <a b> where a is the interrupt number and b represents an
+   encoding of the sense and level information for the interrupt.
+
+Required sub-node:
+ - connector : The "usb-c-connector" attached to the hd3ss3220 chip. The
+   bindings of the connector node are specified in:
+
+	Documentation/devicetree/bindings/connector/usb-connector.txt
+
+Example:
+hd3ss3220@47 {
+	compatible = "ti,hd3ss3220";
+	reg = <0x47>;
+	interrupt-parent = <&gpio6>;
+	interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+
+	usb_con: connector {
+		compatible = "usb-c-connector";
+		label = "USB-C";
+		data-role = "dual";
+	};
+
+	port {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		hd3ss3220_ep: endpoint@0 {
+			reg = <0>;
+			remote-endpoint = <&usb3peri_role_switch>;
+		};
+	};
+};
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [V4,1/7] dt-bindings: usb: hd3ss3220 device tree binding document
@ 2019-04-16  9:38   ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: Biju Das, Greg Kroah-Hartman, Heikki Krogerus, Felipe Balbi,
	linux-usb, devicetree, Simon Horman, Yoshihiro Shimoda,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	linux-renesas-soc

Add device tree binding document for TI HD3SS3220 Type-C DRP port
controller driver.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
V3-->V4
  * No Change.
V2-->V3
  * Added Rob's Reviewed by tag.
V1-->V2
  * Added connector node.
  * updated the example with connector node.
---
 .../devicetree/bindings/usb/ti,hd3ss3220.txt       | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/ti,hd3ss3220.txt

diff --git a/Documentation/devicetree/bindings/usb/ti,hd3ss3220.txt b/Documentation/devicetree/bindings/usb/ti,hd3ss3220.txt
new file mode 100644
index 0000000..7f41400
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/ti,hd3ss3220.txt
@@ -0,0 +1,37 @@
+TI HD3SS3220 TypeC DRP Port Controller.
+
+Required properties:
+ - compatible: Must be "ti,hd3ss3220".
+ - reg: I2C slave address, must be 0x47 or 0x67 based on ADDR pin.
+ - interrupts: <a b> where a is the interrupt number and b represents an
+   encoding of the sense and level information for the interrupt.
+
+Required sub-node:
+ - connector : The "usb-c-connector" attached to the hd3ss3220 chip. The
+   bindings of the connector node are specified in:
+
+	Documentation/devicetree/bindings/connector/usb-connector.txt
+
+Example:
+hd3ss3220@47 {
+	compatible = "ti,hd3ss3220";
+	reg = <0x47>;
+	interrupt-parent = <&gpio6>;
+	interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+
+	usb_con: connector {
+		compatible = "usb-c-connector";
+		label = "USB-C";
+		data-role = "dual";
+	};
+
+	port {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		hd3ss3220_ep: endpoint@0 {
+			reg = <0>;
+			remote-endpoint = <&usb3peri_role_switch>;
+		};
+	};
+};

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V4 2/7] dt-bindings: usb: renesas_usb3: Add renesas,usb-role-switch property
@ 2019-04-16  9:38   ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: Biju Das, Greg Kroah-Hartman, Heikki Krogerus, Felipe Balbi,
	linux-usb, devicetree, Simon Horman, Yoshihiro Shimoda,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	linux-renesas-soc

Add an optional property renesas,usb-role-switch to support
dual role switch for USB Type-C DRP port controller devices
using USB role switch class framework.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 V3-->V4
  * No Change
 V2-->V3
  * Added optional renesas,usb-role-switch property.
 V1-->V2
  * Added usb-role-switch-property
  * Updated the example with usb-role-switch property.
---
 .../devicetree/bindings/usb/renesas_usb3.txt       | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
index 35039e7..f1cb06a 100644
--- a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
+++ b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
@@ -22,6 +22,7 @@ Required properties:
 Optional properties:
   - phys: phandle + phy specifier pair
   - phy-names: must be "usb"
+  - renesas,usb-role-switch: use USB role switch to handle role switch events
 
 Example of R-Car H3 ES1.x:
 	usb3_peri0: usb@ee020000 {
@@ -39,3 +40,24 @@ Example of R-Car H3 ES1.x:
 		interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cpg CPG_MOD 327>;
 	};
+
+Example of RZ/G2E:
+	usb3_peri0: usb@ee020000 {
+		compatible = "renesas,r8a774c0-usb3-peri",
+			     "renesas,rcar-gen3-usb3-peri";
+		reg = <0 0xee020000 0 0x400>;
+		interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cpg CPG_MOD 328>;
+		companion = <&xhci0>;
+		renesas,usb-role-switch;
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			usb3peri_role_switch: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&hd3ss3220_ep>;
+			};
+		};
+	};
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [V4,2/7] dt-bindings: usb: renesas_usb3: Add renesas,usb-role-switch property
@ 2019-04-16  9:38   ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: Biju Das, Greg Kroah-Hartman, Heikki Krogerus, Felipe Balbi,
	linux-usb, devicetree, Simon Horman, Yoshihiro Shimoda,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	linux-renesas-soc

Add an optional property renesas,usb-role-switch to support
dual role switch for USB Type-C DRP port controller devices
using USB role switch class framework.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 V3-->V4
  * No Change
 V2-->V3
  * Added optional renesas,usb-role-switch property.
 V1-->V2
  * Added usb-role-switch-property
  * Updated the example with usb-role-switch property.
---
 .../devicetree/bindings/usb/renesas_usb3.txt       | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
index 35039e7..f1cb06a 100644
--- a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
+++ b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
@@ -22,6 +22,7 @@ Required properties:
 Optional properties:
   - phys: phandle + phy specifier pair
   - phy-names: must be "usb"
+  - renesas,usb-role-switch: use USB role switch to handle role switch events
 
 Example of R-Car H3 ES1.x:
 	usb3_peri0: usb@ee020000 {
@@ -39,3 +40,24 @@ Example of R-Car H3 ES1.x:
 		interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cpg CPG_MOD 327>;
 	};
+
+Example of RZ/G2E:
+	usb3_peri0: usb@ee020000 {
+		compatible = "renesas,r8a774c0-usb3-peri",
+			     "renesas,rcar-gen3-usb3-peri";
+		reg = <0 0xee020000 0 0x400>;
+		interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cpg CPG_MOD 328>;
+		companion = <&xhci0>;
+		renesas,usb-role-switch;
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			usb3peri_role_switch: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&hd3ss3220_ep>;
+			};
+		};
+	};

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V4 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller
@ 2019-04-16  9:38   ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Heikki Krogerus, Chunfeng Yun
  Cc: Biju Das, Greg Kroah-Hartman, Felipe Balbi, linux-usb,
	Simon Horman, Yoshihiro Shimoda, Geert Uytterhoeven,
	Chris Paterson, Fabrizio Castro, linux-renesas-soc

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 <biju.das@bp.renesas.com>
---
 Note: This patch has compilation dependency on 
 https://patchwork.kernel.org/patch/10882555/
 
 V3-->V4
   * Incorporated Chunfeng Yun's review comment
   * Used fwnode API's to get usb role switch handle.
 
 V2-->V3
   * Used the new api "usb_role_switch by node" for getting
     remote endpoint associated with Type-C USB DRP port
     controller devices.
 V1-->V2
   * Driver uses usb role class instead of extcon for dual role switch
     and also handles connect/disconnect events.
---
 drivers/usb/typec/Kconfig     |  10 ++
 drivers/usb/typec/Makefile    |   1 +
 drivers/usb/typec/hd3ss3220.c | 263 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 274 insertions(+)
 create mode 100644 drivers/usb/typec/hd3ss3220.c

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 89d9193..92a3717 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -50,6 +50,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 0000000..e98a38f
--- /dev/null
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * TI HD3SS3220 Type-C DRP Port Controller Driver
+ *
+ * Copyright (C) 2019 Renesas Electronics Corp.
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/usb/role.h>
+#include <linux/irqreturn.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/usb/typec.h>
+#include <linux/delay.h>
+
+#define HD3SS3220_REG_CN_STAT_CTRL	0x09
+#define HD3SS3220_REG_GEN_CTRL		0x0A
+#define HD3SS3220_REG_DEV_REV		0xA0
+
+/* 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_DFP		BIT(6)
+#define HD3SS3220_REG_CN_STAT_CTRL_AS_UFP		BIT(7)
+#define HD3SS3220_REG_CN_STAT_CTRL_TO_ACCESSORY		(BIT(7) | BIT(6))
+#define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS		BIT(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 usb_role_switch	*role_sw;
+	struct typec_port *port;
+	struct typec_capability typec_cap;
+};
+
+static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
+{
+	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
+				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK,
+				  src_pref);
+}
+
+static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220 *hd3ss3220)
+{
+	unsigned int reg_val;
+	enum usb_role attached_state;
+	int ret;
+
+	ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL,
+			  &reg_val);
+	if (ret < 0)
+		return ret;
+
+	switch (reg_val & HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK) {
+	case HD3SS3220_REG_CN_STAT_CTRL_AS_DFP:
+		attached_state = USB_ROLE_HOST;
+	break;
+	case HD3SS3220_REG_CN_STAT_CTRL_AS_UFP:
+		attached_state = USB_ROLE_DEVICE;
+	break;
+	default:
+		attached_state = USB_ROLE_NONE;
+	}
+
+	return attached_state;
+}
+
+static int hd3ss3220_dr_set(const struct typec_capability *cap,
+			    enum typec_data_role role)
+{
+	struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220,
+						   typec_cap);
+	enum usb_role role_val;
+	int pref, ret = 0;
+
+	if (role == TYPEC_HOST) {
+		role_val = USB_ROLE_HOST;
+		pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC;
+	} else {
+		role_val = USB_ROLE_DEVICE;
+		pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK;
+	}
+
+	ret = hd3ss3220_set_source_pref(hd3ss3220, pref);
+	usleep_range(10, 100);
+
+	usb_role_switch_set_role(hd3ss3220->role_sw, role_val);
+	typec_set_data_role(hd3ss3220->port, role);
+
+	return ret;
+}
+
+static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
+{
+	enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
+
+	usb_role_switch_set_role(hd3ss3220->role_sw, role_state);
+	if (role_state == USB_ROLE_NONE)
+		hd3ss3220_set_source_pref(hd3ss3220,
+				HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
+
+	switch (role_state) {
+	case USB_ROLE_HOST:
+		typec_set_data_role(hd3ss3220->port, TYPEC_HOST);
+		break;
+	case USB_ROLE_DEVICE:
+		typec_set_data_role(hd3ss3220->port, TYPEC_DEVICE);
+		break;
+	default:
+		break;
+	}
+}
+
+irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220)
+{
+	int err;
+
+	hd3ss3220_set_role(hd3ss3220);
+	err = regmap_update_bits_base(hd3ss3220->regmap,
+				      HD3SS3220_REG_CN_STAT_CTRL,
+				      HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
+				      HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
+				      NULL, false, true);
+	if (err < 0)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t hd3ss3220_irq_handler(int irq, void *data)
+{
+	struct i2c_client *client = to_i2c_client(data);
+	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
+
+	return hd3ss3220_irq(hd3ss3220);
+}
+
+static const struct regmap_config config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x0A,
+};
+
+static int hd3ss3220_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	struct hd3ss3220 *hd3ss3220;
+	struct fwnode_handle *parent, *child;
+	int ret;
+	unsigned int data;
+
+	hd3ss3220 = devm_kzalloc(&client->dev, sizeof(struct hd3ss3220),
+				 GFP_KERNEL);
+	if (!hd3ss3220)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, hd3ss3220);
+
+	hd3ss3220->dev = &client->dev;
+	hd3ss3220->regmap = devm_regmap_init_i2c(client, &config);
+	if (IS_ERR(hd3ss3220->regmap))
+		return PTR_ERR(hd3ss3220->regmap);
+
+	hd3ss3220_set_source_pref(hd3ss3220,
+				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
+	child = fwnode_graph_get_next_endpoint(dev_fwnode(hd3ss3220->dev),
+					       NULL);
+	parent = fwnode_graph_get_remote_port_parent(child);
+	hd3ss3220->role_sw = fwnode_usb_role_switch_get(parent);
+	if (IS_ERR_OR_NULL(hd3ss3220->role_sw)) {
+		fwnode_handle_put(child);
+		fwnode_handle_put(parent);
+		return PTR_ERR(hd3ss3220->role_sw);
+	}
+
+	fwnode_handle_put(child);
+	fwnode_handle_put(parent);
+
+	hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
+	hd3ss3220->typec_cap.dr_set = hd3ss3220_dr_set;
+	hd3ss3220->typec_cap.type = TYPEC_PORT_DRP;
+	hd3ss3220->typec_cap.data = TYPEC_PORT_DRD;
+
+	hd3ss3220->port = typec_register_port(&client->dev,
+					      &hd3ss3220->typec_cap);
+	if (IS_ERR(hd3ss3220->port))
+		return PTR_ERR(hd3ss3220->port);
+
+	hd3ss3220_set_role(hd3ss3220);
+	if (client->irq > 0) {
+		ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL,
+				  &data);
+		if (ret < 0)
+			goto error;
+
+		if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
+			ret = regmap_write(hd3ss3220->regmap,
+				HD3SS3220_REG_CN_STAT_CTRL,
+				data | HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
+			if (ret < 0)
+				goto error;
+		}
+
+		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+					hd3ss3220_irq_handler,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"hd3ss3220", &client->dev);
+		if (ret)
+			goto error;
+	}
+
+	ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
+	if (ret < 0)
+		goto error;
+
+	dev_info(&client->dev, "probed revision=0x%x\n", ret);
+
+	return 0;
+error:
+	typec_unregister_port(hd3ss3220->port);
+	usb_role_switch_put(hd3ss3220->role_sw);
+
+	return ret;
+}
+
+static int hd3ss3220_remove(struct i2c_client *client)
+{
+	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
+
+	typec_unregister_port(hd3ss3220->port);
+	usb_role_switch_put(hd3ss3220->role_sw);
+
+	return 0;
+}
+
+static const struct of_device_id dev_ids[] = {
+	{ .compatible = "ti,hd3ss3220"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, dev_ids);
+
+static struct i2c_driver hd3ss3220_driver = {
+	.driver = {
+		.name = "hd3ss3220",
+		.of_match_table = of_match_ptr(dev_ids),
+	},
+	.probe = hd3ss3220_probe,
+	.remove =  hd3ss3220_remove,
+};
+
+module_i2c_driver(hd3ss3220_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das@bp.renesas.com>");
+MODULE_DESCRIPTION("TI HD3SS3220 DRP Port Controller Driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [V4,3/7] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller
@ 2019-04-16  9:38   ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Heikki Krogerus, Chunfeng Yun
  Cc: Biju Das, Greg Kroah-Hartman, Felipe Balbi, linux-usb,
	Simon Horman, Yoshihiro Shimoda, Geert Uytterhoeven,
	Chris Paterson, Fabrizio Castro, linux-renesas-soc

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 <biju.das@bp.renesas.com>
---
 Note: This patch has compilation dependency on 
 https://patchwork.kernel.org/patch/10882555/
 
 V3-->V4
   * Incorporated Chunfeng Yun's review comment
   * Used fwnode API's to get usb role switch handle.
 
 V2-->V3
   * Used the new api "usb_role_switch by node" for getting
     remote endpoint associated with Type-C USB DRP port
     controller devices.
 V1-->V2
   * Driver uses usb role class instead of extcon for dual role switch
     and also handles connect/disconnect events.
---
 drivers/usb/typec/Kconfig     |  10 ++
 drivers/usb/typec/Makefile    |   1 +
 drivers/usb/typec/hd3ss3220.c | 263 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 274 insertions(+)
 create mode 100644 drivers/usb/typec/hd3ss3220.c

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 89d9193..92a3717 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -50,6 +50,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 0000000..e98a38f
--- /dev/null
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * TI HD3SS3220 Type-C DRP Port Controller Driver
+ *
+ * Copyright (C) 2019 Renesas Electronics Corp.
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/usb/role.h>
+#include <linux/irqreturn.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/usb/typec.h>
+#include <linux/delay.h>
+
+#define HD3SS3220_REG_CN_STAT_CTRL	0x09
+#define HD3SS3220_REG_GEN_CTRL		0x0A
+#define HD3SS3220_REG_DEV_REV		0xA0
+
+/* 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_DFP		BIT(6)
+#define HD3SS3220_REG_CN_STAT_CTRL_AS_UFP		BIT(7)
+#define HD3SS3220_REG_CN_STAT_CTRL_TO_ACCESSORY		(BIT(7) | BIT(6))
+#define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS		BIT(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 usb_role_switch	*role_sw;
+	struct typec_port *port;
+	struct typec_capability typec_cap;
+};
+
+static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
+{
+	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
+				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK,
+				  src_pref);
+}
+
+static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220 *hd3ss3220)
+{
+	unsigned int reg_val;
+	enum usb_role attached_state;
+	int ret;
+
+	ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL,
+			  &reg_val);
+	if (ret < 0)
+		return ret;
+
+	switch (reg_val & HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK) {
+	case HD3SS3220_REG_CN_STAT_CTRL_AS_DFP:
+		attached_state = USB_ROLE_HOST;
+	break;
+	case HD3SS3220_REG_CN_STAT_CTRL_AS_UFP:
+		attached_state = USB_ROLE_DEVICE;
+	break;
+	default:
+		attached_state = USB_ROLE_NONE;
+	}
+
+	return attached_state;
+}
+
+static int hd3ss3220_dr_set(const struct typec_capability *cap,
+			    enum typec_data_role role)
+{
+	struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220,
+						   typec_cap);
+	enum usb_role role_val;
+	int pref, ret = 0;
+
+	if (role == TYPEC_HOST) {
+		role_val = USB_ROLE_HOST;
+		pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC;
+	} else {
+		role_val = USB_ROLE_DEVICE;
+		pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK;
+	}
+
+	ret = hd3ss3220_set_source_pref(hd3ss3220, pref);
+	usleep_range(10, 100);
+
+	usb_role_switch_set_role(hd3ss3220->role_sw, role_val);
+	typec_set_data_role(hd3ss3220->port, role);
+
+	return ret;
+}
+
+static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
+{
+	enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
+
+	usb_role_switch_set_role(hd3ss3220->role_sw, role_state);
+	if (role_state == USB_ROLE_NONE)
+		hd3ss3220_set_source_pref(hd3ss3220,
+				HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
+
+	switch (role_state) {
+	case USB_ROLE_HOST:
+		typec_set_data_role(hd3ss3220->port, TYPEC_HOST);
+		break;
+	case USB_ROLE_DEVICE:
+		typec_set_data_role(hd3ss3220->port, TYPEC_DEVICE);
+		break;
+	default:
+		break;
+	}
+}
+
+irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220)
+{
+	int err;
+
+	hd3ss3220_set_role(hd3ss3220);
+	err = regmap_update_bits_base(hd3ss3220->regmap,
+				      HD3SS3220_REG_CN_STAT_CTRL,
+				      HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
+				      HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
+				      NULL, false, true);
+	if (err < 0)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t hd3ss3220_irq_handler(int irq, void *data)
+{
+	struct i2c_client *client = to_i2c_client(data);
+	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
+
+	return hd3ss3220_irq(hd3ss3220);
+}
+
+static const struct regmap_config config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x0A,
+};
+
+static int hd3ss3220_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	struct hd3ss3220 *hd3ss3220;
+	struct fwnode_handle *parent, *child;
+	int ret;
+	unsigned int data;
+
+	hd3ss3220 = devm_kzalloc(&client->dev, sizeof(struct hd3ss3220),
+				 GFP_KERNEL);
+	if (!hd3ss3220)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, hd3ss3220);
+
+	hd3ss3220->dev = &client->dev;
+	hd3ss3220->regmap = devm_regmap_init_i2c(client, &config);
+	if (IS_ERR(hd3ss3220->regmap))
+		return PTR_ERR(hd3ss3220->regmap);
+
+	hd3ss3220_set_source_pref(hd3ss3220,
+				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
+	child = fwnode_graph_get_next_endpoint(dev_fwnode(hd3ss3220->dev),
+					       NULL);
+	parent = fwnode_graph_get_remote_port_parent(child);
+	hd3ss3220->role_sw = fwnode_usb_role_switch_get(parent);
+	if (IS_ERR_OR_NULL(hd3ss3220->role_sw)) {
+		fwnode_handle_put(child);
+		fwnode_handle_put(parent);
+		return PTR_ERR(hd3ss3220->role_sw);
+	}
+
+	fwnode_handle_put(child);
+	fwnode_handle_put(parent);
+
+	hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
+	hd3ss3220->typec_cap.dr_set = hd3ss3220_dr_set;
+	hd3ss3220->typec_cap.type = TYPEC_PORT_DRP;
+	hd3ss3220->typec_cap.data = TYPEC_PORT_DRD;
+
+	hd3ss3220->port = typec_register_port(&client->dev,
+					      &hd3ss3220->typec_cap);
+	if (IS_ERR(hd3ss3220->port))
+		return PTR_ERR(hd3ss3220->port);
+
+	hd3ss3220_set_role(hd3ss3220);
+	if (client->irq > 0) {
+		ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL,
+				  &data);
+		if (ret < 0)
+			goto error;
+
+		if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
+			ret = regmap_write(hd3ss3220->regmap,
+				HD3SS3220_REG_CN_STAT_CTRL,
+				data | HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
+			if (ret < 0)
+				goto error;
+		}
+
+		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+					hd3ss3220_irq_handler,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"hd3ss3220", &client->dev);
+		if (ret)
+			goto error;
+	}
+
+	ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
+	if (ret < 0)
+		goto error;
+
+	dev_info(&client->dev, "probed revision=0x%x\n", ret);
+
+	return 0;
+error:
+	typec_unregister_port(hd3ss3220->port);
+	usb_role_switch_put(hd3ss3220->role_sw);
+
+	return ret;
+}
+
+static int hd3ss3220_remove(struct i2c_client *client)
+{
+	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
+
+	typec_unregister_port(hd3ss3220->port);
+	usb_role_switch_put(hd3ss3220->role_sw);
+
+	return 0;
+}
+
+static const struct of_device_id dev_ids[] = {
+	{ .compatible = "ti,hd3ss3220"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, dev_ids);
+
+static struct i2c_driver hd3ss3220_driver = {
+	.driver = {
+		.name = "hd3ss3220",
+		.of_match_table = of_match_ptr(dev_ids),
+	},
+	.probe = hd3ss3220_probe,
+	.remove =  hd3ss3220_remove,
+};
+
+module_i2c_driver(hd3ss3220_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das@bp.renesas.com>");
+MODULE_DESCRIPTION("TI HD3SS3220 DRP Port Controller Driver");
+MODULE_LICENSE("GPL");

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
@ 2019-04-16  9:38   ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Biju Das, Greg Kroah-Hartman, Heikki Krogerus, Yoshihiro Shimoda,
	Simon Horman, Fabrizio Castro, Kees Cook, linux-usb,
	Simon Horman, Geert Uytterhoeven, Chris Paterson,
	linux-renesas-soc

RZ/G2E cat874 board is capable of detecting cable connect and disconnect
events. Add support for renesas_usb3 to receive connect and disconnect
events and support dual-role switch using usb-role-switch framework.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 V3-->V4
   * No Change
 V2-->V3
   * Incorporated Shimoda-san's review comment
     (https://patchwork.kernel.org/patch/10852507/)
   * Used renesas,usb-role-switch property for differentiating USB
     role switch associated with Type-C port controller driver.
 V1-->V2
   * Driver uses usb role clas for handling dual role switch and handling
     connect/disconnect events instead of extcon.
---
 drivers/usb/gadget/udc/renesas_usb3.c | 126 ++++++++++++++++++++++++++++------
 1 file changed, 104 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
index 7dc2485..efee047 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -351,6 +351,8 @@ struct renesas_usb3 {
 	int disabled_count;
 
 	struct usb_request *ep0_req;
+
+	enum usb_role connection_state;
 	u16 test_mode;
 	u8 ep0_buf[USB3_EP0_BUF_SIZE];
 	bool softconnect;
@@ -359,6 +361,7 @@ struct renesas_usb3 {
 	bool extcon_usb;		/* check vbus and set EXTCON_USB */
 	bool forced_b_device;
 	bool start_to_connect;
+	bool usb_role_switch_property;
 };
 
 #define gadget_to_renesas_usb3(_gadget)	\
@@ -644,22 +647,6 @@ static void usb3_disconnect(struct renesas_usb3 *usb3)
 		usb3->driver->disconnect(&usb3->gadget);
 }
 
-static void usb3_check_vbus(struct renesas_usb3 *usb3)
-{
-	if (usb3->workaround_for_vbus) {
-		usb3_connect(usb3);
-	} else {
-		usb3->extcon_usb = !!(usb3_read(usb3, USB3_USB_STA) &
-							USB_STA_VBUS_STA);
-		if (usb3->extcon_usb)
-			usb3_connect(usb3);
-		else
-			usb3_disconnect(usb3);
-
-		schedule_work(&usb3->extcon_work);
-	}
-}
-
 static void renesas_usb3_role_work(struct work_struct *work)
 {
 	struct renesas_usb3 *usb3 =
@@ -699,8 +686,11 @@ static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool a_dev)
 	unsigned long flags;
 
 	spin_lock_irqsave(&usb3->lock, flags);
-	usb3_set_mode_by_role_sw(usb3, host);
-	usb3_vbus_out(usb3, a_dev);
+	if (!usb3->usb_role_switch_property ||
+	    usb3->connection_state != USB_ROLE_NONE) {
+		usb3_set_mode_by_role_sw(usb3, host);
+		usb3_vbus_out(usb3, a_dev);
+	}
 	/* for A-Peripheral or forced B-device mode */
 	if ((!host && a_dev) || usb3->start_to_connect)
 		usb3_connect(usb3);
@@ -724,6 +714,28 @@ static void usb3_check_id(struct renesas_usb3 *usb3)
 	schedule_work(&usb3->extcon_work);
 }
 
+static void usb3_check_vbus(struct renesas_usb3 *usb3)
+{
+	if (usb3->workaround_for_vbus) {
+		if (usb3->usb_role_switch_property) {
+			if (usb3->connection_state == USB_ROLE_DEVICE) {
+				usb3_mode_config(usb3, false, false);
+				usb3_connect(usb3);
+			}
+		} else
+			usb3_connect(usb3);
+	} else {
+		usb3->extcon_usb = !!(usb3_read(usb3, USB3_USB_STA) &
+							USB_STA_VBUS_STA);
+		if (usb3->extcon_usb)
+			usb3_connect(usb3);
+		else
+			usb3_disconnect(usb3);
+
+		schedule_work(&usb3->extcon_work);
+	}
+}
+
 static void renesas_usb3_init_controller(struct renesas_usb3 *usb3)
 {
 	usb3_init_axi_bridge(usb3);
@@ -2343,14 +2355,65 @@ static enum usb_role renesas_usb3_role_switch_get(struct device *dev)
 	return cur_role;
 }
 
-static int renesas_usb3_role_switch_set(struct device *dev,
-					enum usb_role role)
+static void handle_ext_role_switch_states(struct device *dev,
+					    enum usb_role role)
+{
+	struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
+	struct device *host = usb3->host_dev;
+	enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
+
+	switch (role) {
+	case USB_ROLE_NONE:
+		usb3->connection_state = USB_ROLE_NONE;
+		if (usb3->driver)
+			usb3_disconnect(usb3);
+		usb3_vbus_out(usb3, false);
+		break;
+	case USB_ROLE_DEVICE:
+		if (usb3->connection_state == USB_ROLE_NONE) {
+			usb3->connection_state = USB_ROLE_DEVICE;
+			usb3_set_mode(usb3, false);
+			if (usb3->driver)
+				usb3_connect(usb3);
+		} else if (cur_role == USB_ROLE_HOST)  {
+			device_release_driver(host);
+			usb3_set_mode(usb3, false);
+			if (usb3->driver)
+				usb3_connect(usb3);
+		}
+		usb3_vbus_out(usb3, false);
+		break;
+	case USB_ROLE_HOST:
+		if (usb3->connection_state == USB_ROLE_NONE) {
+			if (usb3->driver)
+				usb3_disconnect(usb3);
+
+			usb3->connection_state = USB_ROLE_HOST;
+			usb3_set_mode(usb3, true);
+			usb3_vbus_out(usb3, true);
+			if (device_attach(host) < 0)
+				dev_err(dev, "device_attach(host) failed\n");
+		} else if (cur_role == USB_ROLE_DEVICE) {
+			usb3_disconnect(usb3);
+			/* Must set the mode before device_attach of the host */
+			usb3_set_mode(usb3, true);
+			/* This device_attach() might sleep */
+			if (device_attach(host) < 0)
+				dev_err(dev, "device_attach(host) failed\n");
+		}
+		break;
+	default:
+		break;
+	}
+}
+
+static void handle_role_switch_states(struct device *dev,
+					    enum usb_role role)
 {
 	struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
 	struct device *host = usb3->host_dev;
 	enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
 
-	pm_runtime_get_sync(dev);
 	if (cur_role == USB_ROLE_HOST && role == USB_ROLE_DEVICE) {
 		device_release_driver(host);
 		usb3_set_mode(usb3, false);
@@ -2361,6 +2424,20 @@ static int renesas_usb3_role_switch_set(struct device *dev,
 		if (device_attach(host) < 0)
 			dev_err(dev, "device_attach(host) failed\n");
 	}
+}
+
+static int renesas_usb3_role_switch_set(struct device *dev,
+					enum usb_role role)
+{
+	struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
+
+	pm_runtime_get_sync(dev);
+
+	if (usb3->usb_role_switch_property)
+		handle_ext_role_switch_states(dev, role);
+	else
+		handle_role_switch_states(dev, role);
+
 	pm_runtime_put(dev);
 
 	return 0;
@@ -2650,7 +2727,7 @@ static const unsigned int renesas_usb3_cable[] = {
 	EXTCON_NONE,
 };
 
-static const struct usb_role_switch_desc renesas_usb3_role_switch_desc = {
+static struct usb_role_switch_desc renesas_usb3_role_switch_desc = {
 	.set = renesas_usb3_role_switch_set,
 	.get = renesas_usb3_role_switch_get,
 	.allow_userspace_control = true,
@@ -2741,6 +2818,11 @@ static int renesas_usb3_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_dev_create;
 
+	if (device_property_read_bool(&pdev->dev, "renesas,usb-role-switch")) {
+		usb3->usb_role_switch_property = true;
+		renesas_usb3_role_switch_desc.fwnode = dev_fwnode(&pdev->dev);
+	}
+
 	INIT_WORK(&usb3->role_work, renesas_usb3_role_work);
 	usb3->role_sw = usb_role_switch_register(&pdev->dev,
 					&renesas_usb3_role_switch_desc);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [V4,4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
@ 2019-04-16  9:38   ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Biju Das, Greg Kroah-Hartman, Heikki Krogerus, Yoshihiro Shimoda,
	Simon Horman, Fabrizio Castro, Kees Cook, linux-usb,
	Simon Horman, Geert Uytterhoeven, Chris Paterson,
	linux-renesas-soc

RZ/G2E cat874 board is capable of detecting cable connect and disconnect
events. Add support for renesas_usb3 to receive connect and disconnect
events and support dual-role switch using usb-role-switch framework.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 V3-->V4
   * No Change
 V2-->V3
   * Incorporated Shimoda-san's review comment
     (https://patchwork.kernel.org/patch/10852507/)
   * Used renesas,usb-role-switch property for differentiating USB
     role switch associated with Type-C port controller driver.
 V1-->V2
   * Driver uses usb role clas for handling dual role switch and handling
     connect/disconnect events instead of extcon.
---
 drivers/usb/gadget/udc/renesas_usb3.c | 126 ++++++++++++++++++++++++++++------
 1 file changed, 104 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
index 7dc2485..efee047 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -351,6 +351,8 @@ struct renesas_usb3 {
 	int disabled_count;
 
 	struct usb_request *ep0_req;
+
+	enum usb_role connection_state;
 	u16 test_mode;
 	u8 ep0_buf[USB3_EP0_BUF_SIZE];
 	bool softconnect;
@@ -359,6 +361,7 @@ struct renesas_usb3 {
 	bool extcon_usb;		/* check vbus and set EXTCON_USB */
 	bool forced_b_device;
 	bool start_to_connect;
+	bool usb_role_switch_property;
 };
 
 #define gadget_to_renesas_usb3(_gadget)	\
@@ -644,22 +647,6 @@ static void usb3_disconnect(struct renesas_usb3 *usb3)
 		usb3->driver->disconnect(&usb3->gadget);
 }
 
-static void usb3_check_vbus(struct renesas_usb3 *usb3)
-{
-	if (usb3->workaround_for_vbus) {
-		usb3_connect(usb3);
-	} else {
-		usb3->extcon_usb = !!(usb3_read(usb3, USB3_USB_STA) &
-							USB_STA_VBUS_STA);
-		if (usb3->extcon_usb)
-			usb3_connect(usb3);
-		else
-			usb3_disconnect(usb3);
-
-		schedule_work(&usb3->extcon_work);
-	}
-}
-
 static void renesas_usb3_role_work(struct work_struct *work)
 {
 	struct renesas_usb3 *usb3 =
@@ -699,8 +686,11 @@ static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool a_dev)
 	unsigned long flags;
 
 	spin_lock_irqsave(&usb3->lock, flags);
-	usb3_set_mode_by_role_sw(usb3, host);
-	usb3_vbus_out(usb3, a_dev);
+	if (!usb3->usb_role_switch_property ||
+	    usb3->connection_state != USB_ROLE_NONE) {
+		usb3_set_mode_by_role_sw(usb3, host);
+		usb3_vbus_out(usb3, a_dev);
+	}
 	/* for A-Peripheral or forced B-device mode */
 	if ((!host && a_dev) || usb3->start_to_connect)
 		usb3_connect(usb3);
@@ -724,6 +714,28 @@ static void usb3_check_id(struct renesas_usb3 *usb3)
 	schedule_work(&usb3->extcon_work);
 }
 
+static void usb3_check_vbus(struct renesas_usb3 *usb3)
+{
+	if (usb3->workaround_for_vbus) {
+		if (usb3->usb_role_switch_property) {
+			if (usb3->connection_state == USB_ROLE_DEVICE) {
+				usb3_mode_config(usb3, false, false);
+				usb3_connect(usb3);
+			}
+		} else
+			usb3_connect(usb3);
+	} else {
+		usb3->extcon_usb = !!(usb3_read(usb3, USB3_USB_STA) &
+							USB_STA_VBUS_STA);
+		if (usb3->extcon_usb)
+			usb3_connect(usb3);
+		else
+			usb3_disconnect(usb3);
+
+		schedule_work(&usb3->extcon_work);
+	}
+}
+
 static void renesas_usb3_init_controller(struct renesas_usb3 *usb3)
 {
 	usb3_init_axi_bridge(usb3);
@@ -2343,14 +2355,65 @@ static enum usb_role renesas_usb3_role_switch_get(struct device *dev)
 	return cur_role;
 }
 
-static int renesas_usb3_role_switch_set(struct device *dev,
-					enum usb_role role)
+static void handle_ext_role_switch_states(struct device *dev,
+					    enum usb_role role)
+{
+	struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
+	struct device *host = usb3->host_dev;
+	enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
+
+	switch (role) {
+	case USB_ROLE_NONE:
+		usb3->connection_state = USB_ROLE_NONE;
+		if (usb3->driver)
+			usb3_disconnect(usb3);
+		usb3_vbus_out(usb3, false);
+		break;
+	case USB_ROLE_DEVICE:
+		if (usb3->connection_state == USB_ROLE_NONE) {
+			usb3->connection_state = USB_ROLE_DEVICE;
+			usb3_set_mode(usb3, false);
+			if (usb3->driver)
+				usb3_connect(usb3);
+		} else if (cur_role == USB_ROLE_HOST)  {
+			device_release_driver(host);
+			usb3_set_mode(usb3, false);
+			if (usb3->driver)
+				usb3_connect(usb3);
+		}
+		usb3_vbus_out(usb3, false);
+		break;
+	case USB_ROLE_HOST:
+		if (usb3->connection_state == USB_ROLE_NONE) {
+			if (usb3->driver)
+				usb3_disconnect(usb3);
+
+			usb3->connection_state = USB_ROLE_HOST;
+			usb3_set_mode(usb3, true);
+			usb3_vbus_out(usb3, true);
+			if (device_attach(host) < 0)
+				dev_err(dev, "device_attach(host) failed\n");
+		} else if (cur_role == USB_ROLE_DEVICE) {
+			usb3_disconnect(usb3);
+			/* Must set the mode before device_attach of the host */
+			usb3_set_mode(usb3, true);
+			/* This device_attach() might sleep */
+			if (device_attach(host) < 0)
+				dev_err(dev, "device_attach(host) failed\n");
+		}
+		break;
+	default:
+		break;
+	}
+}
+
+static void handle_role_switch_states(struct device *dev,
+					    enum usb_role role)
 {
 	struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
 	struct device *host = usb3->host_dev;
 	enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
 
-	pm_runtime_get_sync(dev);
 	if (cur_role == USB_ROLE_HOST && role == USB_ROLE_DEVICE) {
 		device_release_driver(host);
 		usb3_set_mode(usb3, false);
@@ -2361,6 +2424,20 @@ static int renesas_usb3_role_switch_set(struct device *dev,
 		if (device_attach(host) < 0)
 			dev_err(dev, "device_attach(host) failed\n");
 	}
+}
+
+static int renesas_usb3_role_switch_set(struct device *dev,
+					enum usb_role role)
+{
+	struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
+
+	pm_runtime_get_sync(dev);
+
+	if (usb3->usb_role_switch_property)
+		handle_ext_role_switch_states(dev, role);
+	else
+		handle_role_switch_states(dev, role);
+
 	pm_runtime_put(dev);
 
 	return 0;
@@ -2650,7 +2727,7 @@ static const unsigned int renesas_usb3_cable[] = {
 	EXTCON_NONE,
 };
 
-static const struct usb_role_switch_desc renesas_usb3_role_switch_desc = {
+static struct usb_role_switch_desc renesas_usb3_role_switch_desc = {
 	.set = renesas_usb3_role_switch_set,
 	.get = renesas_usb3_role_switch_get,
 	.allow_userspace_control = true,
@@ -2741,6 +2818,11 @@ static int renesas_usb3_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_dev_create;
 
+	if (device_property_read_bool(&pdev->dev, "renesas,usb-role-switch")) {
+		usb3->usb_role_switch_property = true;
+		renesas_usb3_role_switch_desc.fwnode = dev_fwnode(&pdev->dev);
+	}
+
 	INIT_WORK(&usb3->role_work, renesas_usb3_role_work);
 	usb3->role_sw = usb_role_switch_register(&pdev->dev,
 					&renesas_usb3_role_switch_desc);

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V4 5/7] arm64: defconfig: enable TYPEC_HD3SS3220 config option
@ 2019-04-16  9:38   ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Biju Das, Greg Kroah-Hartman, Felipe Balbi, linux-usb,
	Simon Horman, Yoshihiro Shimoda, Geert Uytterhoeven,
	Chris Paterson, Fabrizio Castro, linux-renesas-soc

Enable support for the TI HD3SS320 USB Type-C DRP Port controller driver
by turning on CONFIG_TYPEC and CONFIG_TYPEC_HD3SS3220 as modules.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
V3-->V4
  * No change
V2-->V3
  * No change
V1-->V2
  * No change
---
 arch/arm64/configs/defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index d06825f..16cb687 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -578,6 +578,8 @@ CONFIG_USB_ULPI=y
 CONFIG_USB_GADGET=y
 CONFIG_USB_RENESAS_USBHS_UDC=m
 CONFIG_USB_RENESAS_USB3=m
+CONFIG_TYPEC=m
+CONFIG_TYPEC_HD3SS3220=m
 CONFIG_MMC=y
 CONFIG_MMC_BLOCK_MINORS=32
 CONFIG_MMC_ARMMMCI=y
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [V4,5/7] arm64: defconfig: enable TYPEC_HD3SS3220 config option
@ 2019-04-16  9:38   ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Biju Das, Greg Kroah-Hartman, Felipe Balbi, linux-usb,
	Simon Horman, Yoshihiro Shimoda, Geert Uytterhoeven,
	Chris Paterson, Fabrizio Castro, linux-renesas-soc

Enable support for the TI HD3SS320 USB Type-C DRP Port controller driver
by turning on CONFIG_TYPEC and CONFIG_TYPEC_HD3SS3220 as modules.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
V3-->V4
  * No change
V2-->V3
  * No change
V1-->V2
  * No change
---
 arch/arm64/configs/defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index d06825f..16cb687 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -578,6 +578,8 @@ CONFIG_USB_ULPI=y
 CONFIG_USB_GADGET=y
 CONFIG_USB_RENESAS_USBHS_UDC=m
 CONFIG_USB_RENESAS_USB3=m
+CONFIG_TYPEC=m
+CONFIG_TYPEC_HD3SS3220=m
 CONFIG_MMC=y
 CONFIG_MMC_BLOCK_MINORS=32
 CONFIG_MMC_ARMMMCI=y

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V4 6/7] arm64: dts: renesas: r8a774c0-cat874: Enable USB3.0 host/peripheral device node
  2019-04-16  9:38 [PATCH V4 0/7] Add USB3.0 and TI HD3SS3220 driver support Biju Das
                   ` (4 preceding siblings ...)
  2019-04-16  9:38   ` [V4,5/7] " Biju Das
@ 2019-04-16  9:38 ` Biju Das
  2019-04-16  9:38 ` [PATCH V4 7/7] arm64: dts: renesas: r8a774c0-cat874: Enable usb role switch support Biju Das
  6 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: Biju Das, Greg Kroah-Hartman, Heikki Krogerus, Felipe Balbi,
	Simon Horman, Yoshihiro Shimoda, Magnus Damm, linux-renesas-soc,
	devicetree, Geert Uytterhoeven, Chris Paterson, Fabrizio Castro

This patch enables USB3.0 host/peripheral device node for r8a774c0
cat874 board.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
V3-->V4
  * No change
V2-->V3
  * No change
V1-->V2
  * No change
---
 arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts b/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts
index 013a48c..b9ae7db 100644
--- a/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts
+++ b/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts
@@ -134,6 +134,11 @@
 		function = "sdhi0";
 		power-source = <1800>;
 	};
+
+	usb30_pins: usb30 {
+		groups = "usb30", "usb30_id";
+		function = "usb30";
+	};
 };
 
 &rwdt {
@@ -166,3 +171,15 @@
 	renesas,no-otg-pins;
 	status = "okay";
 };
+
+&usb3_peri0 {
+	companion = <&xhci0>;
+	status = "okay";
+};
+
+&xhci0 {
+	pinctrl-0 = <&usb30_pins>;
+	pinctrl-names = "default";
+
+	status = "okay";
+};
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V4 7/7] arm64: dts: renesas: r8a774c0-cat874: Enable usb role switch support
  2019-04-16  9:38 [PATCH V4 0/7] Add USB3.0 and TI HD3SS3220 driver support Biju Das
                   ` (5 preceding siblings ...)
  2019-04-16  9:38 ` [PATCH V4 6/7] arm64: dts: renesas: r8a774c0-cat874: Enable USB3.0 host/peripheral device node Biju Das
@ 2019-04-16  9:38 ` Biju Das
  6 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-16  9:38 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: Biju Das, Greg Kroah-Hartman, Heikki Krogerus, Felipe Balbi,
	Simon Horman, Yoshihiro Shimoda, Magnus Damm, linux-renesas-soc,
	devicetree, Geert Uytterhoeven, Chris Paterson, Fabrizio Castro

This patch enables TI HD3SS3220 device and support usb role switch
for the CAT 874 platform.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
V3-->V4
  * No change
V2-->V3
  * Used "renesas,usb-role-switch" instead of generic "usb-role-switch"
    property
V1-->V2
  * New patch
---
 arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts | 39 +++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts b/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts
index b9ae7db..746826c 100644
--- a/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts
+++ b/arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts
@@ -85,6 +85,34 @@
 	clock-frequency = <48000000>;
 };
 
+&i2c0 {
+	status = "okay";
+	clock-frequency = <100000>;
+
+	hd3ss3220@47 {
+		compatible = "ti,hd3ss3220";
+		reg = <0x47>;
+		interrupt-parent = <&gpio6>;
+		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+
+		usb_con: connector {
+			compatible = "usb-c-connector";
+			label = "USB-C";
+			data-role = "dual";
+		};
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			hd3ss3220_ep: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&usb3peri_role_switch>;
+			};
+		};
+	};
+};
+
 &i2c1 {
 	pinctrl-0 = <&i2c1_pins>;
 	pinctrl-names = "default";
@@ -175,6 +203,17 @@
 &usb3_peri0 {
 	companion = <&xhci0>;
 	status = "okay";
+	renesas,usb-role-switch;
+
+	port {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		usb3peri_role_switch: endpoint@0 {
+			reg = <0>;
+			remote-endpoint = <&hd3ss3220_ep>;
+		};
+	};
 };
 
 &xhci0 {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH V4 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller
@ 2019-04-17  9:57     ` Heikki Krogerus
  0 siblings, 0 replies; 25+ messages in thread
From: Heikki Krogerus @ 2019-04-17  9:57 UTC (permalink / raw)
  To: Biju Das
  Cc: Chunfeng Yun, Greg Kroah-Hartman, Felipe Balbi, linux-usb,
	Simon Horman, Yoshihiro Shimoda, Geert Uytterhoeven,
	Chris Paterson, Fabrizio Castro, linux-renesas-soc

On Tue, Apr 16, 2019 at 10:38:03AM +0100, 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 <biju.das@bp.renesas.com>

It's too bad they call it "port controller" even though it's not Port
Controller Interface compliant. I put a few minor nitpicks below.
Otherwise this looks okay to me, so if there are no other comments:

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  Note: This patch has compilation dependency on 
>  https://patchwork.kernel.org/patch/10882555/
>  
>  V3-->V4
>    * Incorporated Chunfeng Yun's review comment
>    * Used fwnode API's to get usb role switch handle.
>  
>  V2-->V3
>    * Used the new api "usb_role_switch by node" for getting
>      remote endpoint associated with Type-C USB DRP port
>      controller devices.
>  V1-->V2
>    * Driver uses usb role class instead of extcon for dual role switch
>      and also handles connect/disconnect events.
> ---
>  drivers/usb/typec/Kconfig     |  10 ++
>  drivers/usb/typec/Makefile    |   1 +
>  drivers/usb/typec/hd3ss3220.c | 263 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 274 insertions(+)
>  create mode 100644 drivers/usb/typec/hd3ss3220.c
> 
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 89d9193..92a3717 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -50,6 +50,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 0000000..e98a38f
> --- /dev/null
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -0,0 +1,263 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * TI HD3SS3220 Type-C DRP Port Controller Driver
> + *
> + * Copyright (C) 2019 Renesas Electronics Corp.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/usb/role.h>
> +#include <linux/irqreturn.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/usb/typec.h>
> +#include <linux/delay.h>
> +
> +#define HD3SS3220_REG_CN_STAT_CTRL	0x09
> +#define HD3SS3220_REG_GEN_CTRL		0x0A
> +#define HD3SS3220_REG_DEV_REV		0xA0
> +
> +/* 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_DFP		BIT(6)
> +#define HD3SS3220_REG_CN_STAT_CTRL_AS_UFP		BIT(7)
> +#define HD3SS3220_REG_CN_STAT_CTRL_TO_ACCESSORY		(BIT(7) | BIT(6))
> +#define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS		BIT(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 usb_role_switch	*role_sw;
> +	struct typec_port *port;
> +	struct typec_capability typec_cap;
> +};
> +
> +static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
> +{
> +	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> +				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK,
> +				  src_pref);
> +}
> +
> +static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220 *hd3ss3220)
> +{
> +	unsigned int reg_val;
> +	enum usb_role attached_state;
> +	int ret;
> +
> +	ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL,
> +			  &reg_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (reg_val & HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK) {
> +	case HD3SS3220_REG_CN_STAT_CTRL_AS_DFP:
> +		attached_state = USB_ROLE_HOST;
> +	break;
> +	case HD3SS3220_REG_CN_STAT_CTRL_AS_UFP:
> +		attached_state = USB_ROLE_DEVICE;
> +	break;

It looks like your line indentation has collapsed here. I pretty sure
scripts/checkpatch.pl would have found these.

> +	default:
> +		attached_state = USB_ROLE_NONE;
> +	}
> +
> +	return attached_state;
> +}
> +
> +static int hd3ss3220_dr_set(const struct typec_capability *cap,
> +			    enum typec_data_role role)
> +{
> +	struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220,
> +						   typec_cap);
> +	enum usb_role role_val;
> +	int pref, ret = 0;
> +
> +	if (role == TYPEC_HOST) {
> +		role_val = USB_ROLE_HOST;
> +		pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC;
> +	} else {
> +		role_val = USB_ROLE_DEVICE;
> +		pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK;
> +	}
> +
> +	ret = hd3ss3220_set_source_pref(hd3ss3220, pref);
> +	usleep_range(10, 100);
> +
> +	usb_role_switch_set_role(hd3ss3220->role_sw, role_val);
> +	typec_set_data_role(hd3ss3220->port, role);
> +
> +	return ret;
> +}
> +
> +static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
> +{
> +	enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
> +
> +	usb_role_switch_set_role(hd3ss3220->role_sw, role_state);
> +	if (role_state == USB_ROLE_NONE)
> +		hd3ss3220_set_source_pref(hd3ss3220,
> +				HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> +
> +	switch (role_state) {
> +	case USB_ROLE_HOST:
> +		typec_set_data_role(hd3ss3220->port, TYPEC_HOST);
> +		break;
> +	case USB_ROLE_DEVICE:
> +		typec_set_data_role(hd3ss3220->port, TYPEC_DEVICE);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220)
> +{
> +	int err;
> +
> +	hd3ss3220_set_role(hd3ss3220);
> +	err = regmap_update_bits_base(hd3ss3220->regmap,
> +				      HD3SS3220_REG_CN_STAT_CTRL,
> +				      HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
> +				      HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
> +				      NULL, false, true);
> +	if (err < 0)
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t hd3ss3220_irq_handler(int irq, void *data)
> +{
> +	struct i2c_client *client = to_i2c_client(data);
> +	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> +
> +	return hd3ss3220_irq(hd3ss3220);
> +}
> +
> +static const struct regmap_config config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x0A,
> +};
> +
> +static int hd3ss3220_probe(struct i2c_client *client,
> +		const struct i2c_device_id *id)
> +{
> +	struct hd3ss3220 *hd3ss3220;
> +	struct fwnode_handle *parent, *child;
> +	int ret;
> +	unsigned int data;
> +
> +	hd3ss3220 = devm_kzalloc(&client->dev, sizeof(struct hd3ss3220),
> +				 GFP_KERNEL);
> +	if (!hd3ss3220)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, hd3ss3220);
> +
> +	hd3ss3220->dev = &client->dev;
> +	hd3ss3220->regmap = devm_regmap_init_i2c(client, &config);
> +	if (IS_ERR(hd3ss3220->regmap))
> +		return PTR_ERR(hd3ss3220->regmap);
> +
> +	hd3ss3220_set_source_pref(hd3ss3220,
> +				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> +	child = fwnode_graph_get_next_endpoint(dev_fwnode(hd3ss3220->dev),
> +					       NULL);
> +	parent = fwnode_graph_get_remote_port_parent(child);
> +	hd3ss3220->role_sw = fwnode_usb_role_switch_get(parent);
> +	if (IS_ERR_OR_NULL(hd3ss3220->role_sw)) {
> +		fwnode_handle_put(child);
> +		fwnode_handle_put(parent);
> +		return PTR_ERR(hd3ss3220->role_sw);
> +	}
> +
> +	fwnode_handle_put(child);
> +	fwnode_handle_put(parent);
> +
> +	hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> +	hd3ss3220->typec_cap.dr_set = hd3ss3220_dr_set;
> +	hd3ss3220->typec_cap.type = TYPEC_PORT_DRP;
> +	hd3ss3220->typec_cap.data = TYPEC_PORT_DRD;
> +
> +	hd3ss3220->port = typec_register_port(&client->dev,
> +					      &hd3ss3220->typec_cap);
> +	if (IS_ERR(hd3ss3220->port))
> +		return PTR_ERR(hd3ss3220->port);
> +
> +	hd3ss3220_set_role(hd3ss3220);
> +	if (client->irq > 0) {
> +		ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL,
> +				  &data);
> +		if (ret < 0)
> +			goto error;
> +
> +		if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
> +			ret = regmap_write(hd3ss3220->regmap,
> +				HD3SS3220_REG_CN_STAT_CTRL,
> +				data | HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
> +			if (ret < 0)
> +				goto error;
> +		}
> +		if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
> +			ret = regmap_write(hd3ss3220->regmap,
> +				HD3SS3220_REG_CN_STAT_CTRL,
> +				data | HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
> +			if (ret < 0)
> +				goto error;
> +		}
> +
> +		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					hd3ss3220_irq_handler,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					"hd3ss3220", &client->dev);
> +		if (ret)
> +			goto error;
> +	}

I would move the above if (client->irq > 0) case to it's own function.

> +	ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
> +	if (ret < 0)
> +		goto error;
> +
> +	dev_info(&client->dev, "probed revision=0x%x\n", ret);
> +
> +	return 0;
> +error:
> +	typec_unregister_port(hd3ss3220->port);
> +	usb_role_switch_put(hd3ss3220->role_sw);
> +
> +	return ret;
> +}
> +
> +static int hd3ss3220_remove(struct i2c_client *client)
> +{
> +	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> +
> +	typec_unregister_port(hd3ss3220->port);
> +	usb_role_switch_put(hd3ss3220->role_sw);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id dev_ids[] = {
> +	{ .compatible = "ti,hd3ss3220"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, dev_ids);
> +
> +static struct i2c_driver hd3ss3220_driver = {
> +	.driver = {
> +		.name = "hd3ss3220",
> +		.of_match_table = of_match_ptr(dev_ids),
> +	},
> +	.probe = hd3ss3220_probe,
> +	.remove =  hd3ss3220_remove,
> +};
> +
> +module_i2c_driver(hd3ss3220_driver);
> +
> +MODULE_AUTHOR("Biju Das <biju.das@bp.renesas.com>");
> +MODULE_DESCRIPTION("TI HD3SS3220 DRP Port Controller Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.7.4

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [V4,3/7] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller
@ 2019-04-17  9:57     ` Heikki Krogerus
  0 siblings, 0 replies; 25+ messages in thread
From: Heikki Krogerus @ 2019-04-17  9:57 UTC (permalink / raw)
  To: Biju Das
  Cc: Chunfeng Yun, Greg Kroah-Hartman, Felipe Balbi, linux-usb,
	Simon Horman, Yoshihiro Shimoda, Geert Uytterhoeven,
	Chris Paterson, Fabrizio Castro, linux-renesas-soc

On Tue, Apr 16, 2019 at 10:38:03AM +0100, 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 <biju.das@bp.renesas.com>

It's too bad they call it "port controller" even though it's not Port
Controller Interface compliant. I put a few minor nitpicks below.
Otherwise this looks okay to me, so if there are no other comments:

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  Note: This patch has compilation dependency on 
>  https://patchwork.kernel.org/patch/10882555/
>  
>  V3-->V4
>    * Incorporated Chunfeng Yun's review comment
>    * Used fwnode API's to get usb role switch handle.
>  
>  V2-->V3
>    * Used the new api "usb_role_switch by node" for getting
>      remote endpoint associated with Type-C USB DRP port
>      controller devices.
>  V1-->V2
>    * Driver uses usb role class instead of extcon for dual role switch
>      and also handles connect/disconnect events.
> ---
>  drivers/usb/typec/Kconfig     |  10 ++
>  drivers/usb/typec/Makefile    |   1 +
>  drivers/usb/typec/hd3ss3220.c | 263 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 274 insertions(+)
>  create mode 100644 drivers/usb/typec/hd3ss3220.c
> 
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 89d9193..92a3717 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -50,6 +50,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 0000000..e98a38f
> --- /dev/null
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -0,0 +1,263 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * TI HD3SS3220 Type-C DRP Port Controller Driver
> + *
> + * Copyright (C) 2019 Renesas Electronics Corp.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/usb/role.h>
> +#include <linux/irqreturn.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/usb/typec.h>
> +#include <linux/delay.h>
> +
> +#define HD3SS3220_REG_CN_STAT_CTRL	0x09
> +#define HD3SS3220_REG_GEN_CTRL		0x0A
> +#define HD3SS3220_REG_DEV_REV		0xA0
> +
> +/* 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_DFP		BIT(6)
> +#define HD3SS3220_REG_CN_STAT_CTRL_AS_UFP		BIT(7)
> +#define HD3SS3220_REG_CN_STAT_CTRL_TO_ACCESSORY		(BIT(7) | BIT(6))
> +#define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS		BIT(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 usb_role_switch	*role_sw;
> +	struct typec_port *port;
> +	struct typec_capability typec_cap;
> +};
> +
> +static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
> +{
> +	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> +				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK,
> +				  src_pref);
> +}
> +
> +static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220 *hd3ss3220)
> +{
> +	unsigned int reg_val;
> +	enum usb_role attached_state;
> +	int ret;
> +
> +	ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL,
> +			  &reg_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (reg_val & HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK) {
> +	case HD3SS3220_REG_CN_STAT_CTRL_AS_DFP:
> +		attached_state = USB_ROLE_HOST;
> +	break;
> +	case HD3SS3220_REG_CN_STAT_CTRL_AS_UFP:
> +		attached_state = USB_ROLE_DEVICE;
> +	break;

It looks like your line indentation has collapsed here. I pretty sure
scripts/checkpatch.pl would have found these.

> +	default:
> +		attached_state = USB_ROLE_NONE;
> +	}
> +
> +	return attached_state;
> +}
> +
> +static int hd3ss3220_dr_set(const struct typec_capability *cap,
> +			    enum typec_data_role role)
> +{
> +	struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220,
> +						   typec_cap);
> +	enum usb_role role_val;
> +	int pref, ret = 0;
> +
> +	if (role == TYPEC_HOST) {
> +		role_val = USB_ROLE_HOST;
> +		pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC;
> +	} else {
> +		role_val = USB_ROLE_DEVICE;
> +		pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK;
> +	}
> +
> +	ret = hd3ss3220_set_source_pref(hd3ss3220, pref);
> +	usleep_range(10, 100);
> +
> +	usb_role_switch_set_role(hd3ss3220->role_sw, role_val);
> +	typec_set_data_role(hd3ss3220->port, role);
> +
> +	return ret;
> +}
> +
> +static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
> +{
> +	enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
> +
> +	usb_role_switch_set_role(hd3ss3220->role_sw, role_state);
> +	if (role_state == USB_ROLE_NONE)
> +		hd3ss3220_set_source_pref(hd3ss3220,
> +				HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> +
> +	switch (role_state) {
> +	case USB_ROLE_HOST:
> +		typec_set_data_role(hd3ss3220->port, TYPEC_HOST);
> +		break;
> +	case USB_ROLE_DEVICE:
> +		typec_set_data_role(hd3ss3220->port, TYPEC_DEVICE);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220)
> +{
> +	int err;
> +
> +	hd3ss3220_set_role(hd3ss3220);
> +	err = regmap_update_bits_base(hd3ss3220->regmap,
> +				      HD3SS3220_REG_CN_STAT_CTRL,
> +				      HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
> +				      HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
> +				      NULL, false, true);
> +	if (err < 0)
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t hd3ss3220_irq_handler(int irq, void *data)
> +{
> +	struct i2c_client *client = to_i2c_client(data);
> +	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> +
> +	return hd3ss3220_irq(hd3ss3220);
> +}
> +
> +static const struct regmap_config config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x0A,
> +};
> +
> +static int hd3ss3220_probe(struct i2c_client *client,
> +		const struct i2c_device_id *id)
> +{
> +	struct hd3ss3220 *hd3ss3220;
> +	struct fwnode_handle *parent, *child;
> +	int ret;
> +	unsigned int data;
> +
> +	hd3ss3220 = devm_kzalloc(&client->dev, sizeof(struct hd3ss3220),
> +				 GFP_KERNEL);
> +	if (!hd3ss3220)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, hd3ss3220);
> +
> +	hd3ss3220->dev = &client->dev;
> +	hd3ss3220->regmap = devm_regmap_init_i2c(client, &config);
> +	if (IS_ERR(hd3ss3220->regmap))
> +		return PTR_ERR(hd3ss3220->regmap);
> +
> +	hd3ss3220_set_source_pref(hd3ss3220,
> +				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> +	child = fwnode_graph_get_next_endpoint(dev_fwnode(hd3ss3220->dev),
> +					       NULL);
> +	parent = fwnode_graph_get_remote_port_parent(child);
> +	hd3ss3220->role_sw = fwnode_usb_role_switch_get(parent);
> +	if (IS_ERR_OR_NULL(hd3ss3220->role_sw)) {
> +		fwnode_handle_put(child);
> +		fwnode_handle_put(parent);
> +		return PTR_ERR(hd3ss3220->role_sw);
> +	}
> +
> +	fwnode_handle_put(child);
> +	fwnode_handle_put(parent);
> +
> +	hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> +	hd3ss3220->typec_cap.dr_set = hd3ss3220_dr_set;
> +	hd3ss3220->typec_cap.type = TYPEC_PORT_DRP;
> +	hd3ss3220->typec_cap.data = TYPEC_PORT_DRD;
> +
> +	hd3ss3220->port = typec_register_port(&client->dev,
> +					      &hd3ss3220->typec_cap);
> +	if (IS_ERR(hd3ss3220->port))
> +		return PTR_ERR(hd3ss3220->port);
> +
> +	hd3ss3220_set_role(hd3ss3220);
> +	if (client->irq > 0) {
> +		ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL,
> +				  &data);
> +		if (ret < 0)
> +			goto error;
> +
> +		if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
> +			ret = regmap_write(hd3ss3220->regmap,
> +				HD3SS3220_REG_CN_STAT_CTRL,
> +				data | HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
> +			if (ret < 0)
> +				goto error;
> +		}
> +		if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
> +			ret = regmap_write(hd3ss3220->regmap,
> +				HD3SS3220_REG_CN_STAT_CTRL,
> +				data | HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
> +			if (ret < 0)
> +				goto error;
> +		}
> +
> +		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					hd3ss3220_irq_handler,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					"hd3ss3220", &client->dev);
> +		if (ret)
> +			goto error;
> +	}

I would move the above if (client->irq > 0) case to it's own function.

> +	ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
> +	if (ret < 0)
> +		goto error;
> +
> +	dev_info(&client->dev, "probed revision=0x%x\n", ret);
> +
> +	return 0;
> +error:
> +	typec_unregister_port(hd3ss3220->port);
> +	usb_role_switch_put(hd3ss3220->role_sw);
> +
> +	return ret;
> +}
> +
> +static int hd3ss3220_remove(struct i2c_client *client)
> +{
> +	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> +
> +	typec_unregister_port(hd3ss3220->port);
> +	usb_role_switch_put(hd3ss3220->role_sw);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id dev_ids[] = {
> +	{ .compatible = "ti,hd3ss3220"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, dev_ids);
> +
> +static struct i2c_driver hd3ss3220_driver = {
> +	.driver = {
> +		.name = "hd3ss3220",
> +		.of_match_table = of_match_ptr(dev_ids),
> +	},
> +	.probe = hd3ss3220_probe,
> +	.remove =  hd3ss3220_remove,
> +};
> +
> +module_i2c_driver(hd3ss3220_driver);
> +
> +MODULE_AUTHOR("Biju Das <biju.das@bp.renesas.com>");
> +MODULE_DESCRIPTION("TI HD3SS3220 DRP Port Controller Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.7.4

thanks,

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH V4 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller
@ 2019-04-17 11:07       ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-17 11:07 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Chunfeng Yun, Greg Kroah-Hartman, Felipe Balbi, linux-usb,
	Simon Horman, Yoshihiro Shimoda, Geert Uytterhoeven,
	Chris Paterson, Fabrizio Castro, linux-renesas-soc

Hi Heikki,

Thanks for the feedback.

> Subject: Re: [PATCH V4 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C
> DRP port controller
> 
> On Tue, Apr 16, 2019 at 10:38:03AM +0100, 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 <biju.das@bp.renesas.com>
> 
> It's too bad they call it "port controller" even though it's not Port Controller
> Interface compliant. I put a few minor nitpicks below.
> Otherwise this looks okay to me, so if there are no other comments:
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> > ---
> >  Note: This patch has compilation dependency on
> > https://patchwork.kernel.org/patch/10882555/
> >
> >  V3-->V4
> >    * Incorporated Chunfeng Yun's review comment
> >    * Used fwnode API's to get usb role switch handle.
> >
> >  V2-->V3
> >    * Used the new api "usb_role_switch by node" for getting
> >      remote endpoint associated with Type-C USB DRP port
> >      controller devices.
> >  V1-->V2
> >    * Driver uses usb role class instead of extcon for dual role switch
> >      and also handles connect/disconnect events.
> > ---
> >  drivers/usb/typec/Kconfig     |  10 ++
> >  drivers/usb/typec/Makefile    |   1 +
> >  drivers/usb/typec/hd3ss3220.c | 263
> > ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 274 insertions(+)
> >  create mode 100644 drivers/usb/typec/hd3ss3220.c
> >
> > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > index 89d9193..92a3717 100644
> > --- a/drivers/usb/typec/Kconfig
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -50,6 +50,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
> > 0000000..e98a38f
> > --- /dev/null
> > +++ b/drivers/usb/typec/hd3ss3220.c
> > @@ -0,0 +1,263 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * TI HD3SS3220 Type-C DRP Port Controller Driver
> > + *
> > + * Copyright (C) 2019 Renesas Electronics Corp.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/usb/role.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb/typec.h>
> > +#include <linux/delay.h>
> > +
> > +#define HD3SS3220_REG_CN_STAT_CTRL	0x09
> > +#define HD3SS3220_REG_GEN_CTRL		0x0A
> > +#define HD3SS3220_REG_DEV_REV		0xA0
> > +
> > +/* 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_DFP		BIT(6)
> > +#define HD3SS3220_REG_CN_STAT_CTRL_AS_UFP		BIT(7)
> > +#define HD3SS3220_REG_CN_STAT_CTRL_TO_ACCESSORY
> 	(BIT(7) | BIT(6))
> > +#define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS		BIT(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 usb_role_switch	*role_sw;
> > +	struct typec_port *port;
> > +	struct typec_capability typec_cap;
> > +};
> > +
> > +static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int
> > +src_pref) {
> > +	return regmap_update_bits(hd3ss3220->regmap,
> HD3SS3220_REG_GEN_CTRL,
> > +
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK,
> > +				  src_pref);
> > +}
> > +
> > +static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220
> > +*hd3ss3220) {
> > +	unsigned int reg_val;
> > +	enum usb_role attached_state;
> > +	int ret;
> > +
> > +	ret = regmap_read(hd3ss3220->regmap,
> HD3SS3220_REG_CN_STAT_CTRL,
> > +			  &reg_val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	switch (reg_val &
> HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK) {
> > +	case HD3SS3220_REG_CN_STAT_CTRL_AS_DFP:
> > +		attached_state = USB_ROLE_HOST;
> > +	break;
> > +	case HD3SS3220_REG_CN_STAT_CTRL_AS_UFP:
> > +		attached_state = USB_ROLE_DEVICE;
> > +	break;
> 
> It looks like your line indentation has collapsed here. I pretty sure
> scripts/checkpatch.pl would have found these.

OK. Will fix this in V5. I have ran Check patch before sending the patch and it didn't complain this.

> > +	default:
> > +		attached_state = USB_ROLE_NONE;
> > +	}
> > +
> > +	return attached_state;
> > +}
> > +
> > +static int hd3ss3220_dr_set(const struct typec_capability *cap,
> > +			    enum typec_data_role role)
> > +{
> > +	struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220,
> > +						   typec_cap);
> > +	enum usb_role role_val;
> > +	int pref, ret = 0;
> > +
> > +	if (role == TYPEC_HOST) {
> > +		role_val = USB_ROLE_HOST;
> > +		pref =
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC;
> > +	} else {
> > +		role_val = USB_ROLE_DEVICE;
> > +		pref =
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK;
> > +	}
> > +
> > +	ret = hd3ss3220_set_source_pref(hd3ss3220, pref);
> > +	usleep_range(10, 100);
> > +
> > +	usb_role_switch_set_role(hd3ss3220->role_sw, role_val);
> > +	typec_set_data_role(hd3ss3220->port, role);
> > +
> > +	return ret;
> > +}
> > +
> > +static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220) {
> > +	enum usb_role role_state =
> hd3ss3220_get_attached_state(hd3ss3220);
> > +
> > +	usb_role_switch_set_role(hd3ss3220->role_sw, role_state);
> > +	if (role_state == USB_ROLE_NONE)
> > +		hd3ss3220_set_source_pref(hd3ss3220,
> > +
> 	HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> > +
> > +	switch (role_state) {
> > +	case USB_ROLE_HOST:
> > +		typec_set_data_role(hd3ss3220->port, TYPEC_HOST);
> > +		break;
> > +	case USB_ROLE_DEVICE:
> > +		typec_set_data_role(hd3ss3220->port, TYPEC_DEVICE);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> > +irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220) {
> > +	int err;
> > +
> > +	hd3ss3220_set_role(hd3ss3220);
> > +	err = regmap_update_bits_base(hd3ss3220->regmap,
> > +				      HD3SS3220_REG_CN_STAT_CTRL,
> > +
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
> > +
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
> > +				      NULL, false, true);
> > +	if (err < 0)
> > +		return IRQ_NONE;
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t hd3ss3220_irq_handler(int irq, void *data) {
> > +	struct i2c_client *client = to_i2c_client(data);
> > +	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> > +
> > +	return hd3ss3220_irq(hd3ss3220);
> > +}
> > +
> > +static const struct regmap_config config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = 0x0A,
> > +};
> > +
> > +static int hd3ss3220_probe(struct i2c_client *client,
> > +		const struct i2c_device_id *id)
> > +{
> > +	struct hd3ss3220 *hd3ss3220;
> > +	struct fwnode_handle *parent, *child;
> > +	int ret;
> > +	unsigned int data;
> > +
> > +	hd3ss3220 = devm_kzalloc(&client->dev, sizeof(struct hd3ss3220),
> > +				 GFP_KERNEL);
> > +	if (!hd3ss3220)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(client, hd3ss3220);
> > +
> > +	hd3ss3220->dev = &client->dev;
> > +	hd3ss3220->regmap = devm_regmap_init_i2c(client, &config);
> > +	if (IS_ERR(hd3ss3220->regmap))
> > +		return PTR_ERR(hd3ss3220->regmap);
> > +
> > +	hd3ss3220_set_source_pref(hd3ss3220,
> > +
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> > +	child = fwnode_graph_get_next_endpoint(dev_fwnode(hd3ss3220-
> >dev),
> > +					       NULL);
> > +	parent = fwnode_graph_get_remote_port_parent(child);
> > +	hd3ss3220->role_sw = fwnode_usb_role_switch_get(parent);
> > +	if (IS_ERR_OR_NULL(hd3ss3220->role_sw)) {
> > +		fwnode_handle_put(child);
> > +		fwnode_handle_put(parent);
> > +		return PTR_ERR(hd3ss3220->role_sw);
> > +	}
> > +
> > +	fwnode_handle_put(child);
> > +	fwnode_handle_put(parent);
> > +
> > +	hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> > +	hd3ss3220->typec_cap.dr_set = hd3ss3220_dr_set;
> > +	hd3ss3220->typec_cap.type = TYPEC_PORT_DRP;
> > +	hd3ss3220->typec_cap.data = TYPEC_PORT_DRD;
> > +
> > +	hd3ss3220->port = typec_register_port(&client->dev,
> > +					      &hd3ss3220->typec_cap);
> > +	if (IS_ERR(hd3ss3220->port))
> > +		return PTR_ERR(hd3ss3220->port);
> > +
> > +	hd3ss3220_set_role(hd3ss3220);
> > +	if (client->irq > 0) {
> > +		ret = regmap_read(hd3ss3220->regmap,
> HD3SS3220_REG_CN_STAT_CTRL,
> > +				  &data);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
> > +			ret = regmap_write(hd3ss3220->regmap,
> > +				HD3SS3220_REG_CN_STAT_CTRL,
> > +				data |
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
> > +			if (ret < 0)
> > +				goto error;
> > +		}
> > +		if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
> > +			ret = regmap_write(hd3ss3220->regmap,
> > +				HD3SS3220_REG_CN_STAT_CTRL,
> > +				data |
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
> > +			if (ret < 0)
> > +				goto error;
> > +		}
> > +
> > +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> NULL,
> > +					hd3ss3220_irq_handler,
> > +					IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT,
> > +					"hd3ss3220", &client->dev);
> > +		if (ret)
> > +			goto error;
> > +	}
> 
> I would move the above if (client->irq > 0) case to it's own function.

OK. Will fix this in V5.

Regards,
Biju

> > +	ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
> > +	if (ret < 0)
> > +		goto error;
> > +
> > +	dev_info(&client->dev, "probed revision=0x%x\n", ret);
> > +
> > +	return 0;
> > +error:
> > +	typec_unregister_port(hd3ss3220->port);
> > +	usb_role_switch_put(hd3ss3220->role_sw);
> > +
> > +	return ret;
> > +}
> > +
> > +static int hd3ss3220_remove(struct i2c_client *client) {
> > +	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> > +
> > +	typec_unregister_port(hd3ss3220->port);
> > +	usb_role_switch_put(hd3ss3220->role_sw);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id dev_ids[] = {
> > +	{ .compatible = "ti,hd3ss3220"},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, dev_ids);
> > +
> > +static struct i2c_driver hd3ss3220_driver = {
> > +	.driver = {
> > +		.name = "hd3ss3220",
> > +		.of_match_table = of_match_ptr(dev_ids),
> > +	},
> > +	.probe = hd3ss3220_probe,
> > +	.remove =  hd3ss3220_remove,
> > +};
> > +
> > +module_i2c_driver(hd3ss3220_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das@bp.renesas.com>");
> > +MODULE_DESCRIPTION("TI HD3SS3220 DRP Port Controller Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.7.4
> 
> thanks,
> 
> --
> heikki

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [V4,3/7] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller
@ 2019-04-17 11:07       ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-17 11:07 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Chunfeng Yun, Greg Kroah-Hartman, Felipe Balbi, linux-usb,
	Simon Horman, Yoshihiro Shimoda, Geert Uytterhoeven,
	Chris Paterson, Fabrizio Castro, linux-renesas-soc

Hi Heikki,

Thanks for the feedback.

> Subject: Re: [PATCH V4 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C
> DRP port controller
> 
> On Tue, Apr 16, 2019 at 10:38:03AM +0100, 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 <biju.das@bp.renesas.com>
> 
> It's too bad they call it "port controller" even though it's not Port Controller
> Interface compliant. I put a few minor nitpicks below.
> Otherwise this looks okay to me, so if there are no other comments:
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> > ---
> >  Note: This patch has compilation dependency on
> > https://patchwork.kernel.org/patch/10882555/
> >
> >  V3-->V4
> >    * Incorporated Chunfeng Yun's review comment
> >    * Used fwnode API's to get usb role switch handle.
> >
> >  V2-->V3
> >    * Used the new api "usb_role_switch by node" for getting
> >      remote endpoint associated with Type-C USB DRP port
> >      controller devices.
> >  V1-->V2
> >    * Driver uses usb role class instead of extcon for dual role switch
> >      and also handles connect/disconnect events.
> > ---
> >  drivers/usb/typec/Kconfig     |  10 ++
> >  drivers/usb/typec/Makefile    |   1 +
> >  drivers/usb/typec/hd3ss3220.c | 263
> > ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 274 insertions(+)
> >  create mode 100644 drivers/usb/typec/hd3ss3220.c
> >
> > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > index 89d9193..92a3717 100644
> > --- a/drivers/usb/typec/Kconfig
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -50,6 +50,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
> > 0000000..e98a38f
> > --- /dev/null
> > +++ b/drivers/usb/typec/hd3ss3220.c
> > @@ -0,0 +1,263 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * TI HD3SS3220 Type-C DRP Port Controller Driver
> > + *
> > + * Copyright (C) 2019 Renesas Electronics Corp.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/usb/role.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb/typec.h>
> > +#include <linux/delay.h>
> > +
> > +#define HD3SS3220_REG_CN_STAT_CTRL	0x09
> > +#define HD3SS3220_REG_GEN_CTRL		0x0A
> > +#define HD3SS3220_REG_DEV_REV		0xA0
> > +
> > +/* 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_DFP		BIT(6)
> > +#define HD3SS3220_REG_CN_STAT_CTRL_AS_UFP		BIT(7)
> > +#define HD3SS3220_REG_CN_STAT_CTRL_TO_ACCESSORY
> 	(BIT(7) | BIT(6))
> > +#define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS		BIT(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 usb_role_switch	*role_sw;
> > +	struct typec_port *port;
> > +	struct typec_capability typec_cap;
> > +};
> > +
> > +static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int
> > +src_pref) {
> > +	return regmap_update_bits(hd3ss3220->regmap,
> HD3SS3220_REG_GEN_CTRL,
> > +
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK,
> > +				  src_pref);
> > +}
> > +
> > +static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220
> > +*hd3ss3220) {
> > +	unsigned int reg_val;
> > +	enum usb_role attached_state;
> > +	int ret;
> > +
> > +	ret = regmap_read(hd3ss3220->regmap,
> HD3SS3220_REG_CN_STAT_CTRL,
> > +			  &reg_val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	switch (reg_val &
> HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK) {
> > +	case HD3SS3220_REG_CN_STAT_CTRL_AS_DFP:
> > +		attached_state = USB_ROLE_HOST;
> > +	break;
> > +	case HD3SS3220_REG_CN_STAT_CTRL_AS_UFP:
> > +		attached_state = USB_ROLE_DEVICE;
> > +	break;
> 
> It looks like your line indentation has collapsed here. I pretty sure
> scripts/checkpatch.pl would have found these.

OK. Will fix this in V5. I have ran Check patch before sending the patch and it didn't complain this.

> > +	default:
> > +		attached_state = USB_ROLE_NONE;
> > +	}
> > +
> > +	return attached_state;
> > +}
> > +
> > +static int hd3ss3220_dr_set(const struct typec_capability *cap,
> > +			    enum typec_data_role role)
> > +{
> > +	struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220,
> > +						   typec_cap);
> > +	enum usb_role role_val;
> > +	int pref, ret = 0;
> > +
> > +	if (role == TYPEC_HOST) {
> > +		role_val = USB_ROLE_HOST;
> > +		pref =
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC;
> > +	} else {
> > +		role_val = USB_ROLE_DEVICE;
> > +		pref =
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK;
> > +	}
> > +
> > +	ret = hd3ss3220_set_source_pref(hd3ss3220, pref);
> > +	usleep_range(10, 100);
> > +
> > +	usb_role_switch_set_role(hd3ss3220->role_sw, role_val);
> > +	typec_set_data_role(hd3ss3220->port, role);
> > +
> > +	return ret;
> > +}
> > +
> > +static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220) {
> > +	enum usb_role role_state =
> hd3ss3220_get_attached_state(hd3ss3220);
> > +
> > +	usb_role_switch_set_role(hd3ss3220->role_sw, role_state);
> > +	if (role_state == USB_ROLE_NONE)
> > +		hd3ss3220_set_source_pref(hd3ss3220,
> > +
> 	HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> > +
> > +	switch (role_state) {
> > +	case USB_ROLE_HOST:
> > +		typec_set_data_role(hd3ss3220->port, TYPEC_HOST);
> > +		break;
> > +	case USB_ROLE_DEVICE:
> > +		typec_set_data_role(hd3ss3220->port, TYPEC_DEVICE);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> > +irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220) {
> > +	int err;
> > +
> > +	hd3ss3220_set_role(hd3ss3220);
> > +	err = regmap_update_bits_base(hd3ss3220->regmap,
> > +				      HD3SS3220_REG_CN_STAT_CTRL,
> > +
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
> > +
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
> > +				      NULL, false, true);
> > +	if (err < 0)
> > +		return IRQ_NONE;
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t hd3ss3220_irq_handler(int irq, void *data) {
> > +	struct i2c_client *client = to_i2c_client(data);
> > +	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> > +
> > +	return hd3ss3220_irq(hd3ss3220);
> > +}
> > +
> > +static const struct regmap_config config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = 0x0A,
> > +};
> > +
> > +static int hd3ss3220_probe(struct i2c_client *client,
> > +		const struct i2c_device_id *id)
> > +{
> > +	struct hd3ss3220 *hd3ss3220;
> > +	struct fwnode_handle *parent, *child;
> > +	int ret;
> > +	unsigned int data;
> > +
> > +	hd3ss3220 = devm_kzalloc(&client->dev, sizeof(struct hd3ss3220),
> > +				 GFP_KERNEL);
> > +	if (!hd3ss3220)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(client, hd3ss3220);
> > +
> > +	hd3ss3220->dev = &client->dev;
> > +	hd3ss3220->regmap = devm_regmap_init_i2c(client, &config);
> > +	if (IS_ERR(hd3ss3220->regmap))
> > +		return PTR_ERR(hd3ss3220->regmap);
> > +
> > +	hd3ss3220_set_source_pref(hd3ss3220,
> > +
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> > +	child = fwnode_graph_get_next_endpoint(dev_fwnode(hd3ss3220-
> >dev),
> > +					       NULL);
> > +	parent = fwnode_graph_get_remote_port_parent(child);
> > +	hd3ss3220->role_sw = fwnode_usb_role_switch_get(parent);
> > +	if (IS_ERR_OR_NULL(hd3ss3220->role_sw)) {
> > +		fwnode_handle_put(child);
> > +		fwnode_handle_put(parent);
> > +		return PTR_ERR(hd3ss3220->role_sw);
> > +	}
> > +
> > +	fwnode_handle_put(child);
> > +	fwnode_handle_put(parent);
> > +
> > +	hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> > +	hd3ss3220->typec_cap.dr_set = hd3ss3220_dr_set;
> > +	hd3ss3220->typec_cap.type = TYPEC_PORT_DRP;
> > +	hd3ss3220->typec_cap.data = TYPEC_PORT_DRD;
> > +
> > +	hd3ss3220->port = typec_register_port(&client->dev,
> > +					      &hd3ss3220->typec_cap);
> > +	if (IS_ERR(hd3ss3220->port))
> > +		return PTR_ERR(hd3ss3220->port);
> > +
> > +	hd3ss3220_set_role(hd3ss3220);
> > +	if (client->irq > 0) {
> > +		ret = regmap_read(hd3ss3220->regmap,
> HD3SS3220_REG_CN_STAT_CTRL,
> > +				  &data);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
> > +			ret = regmap_write(hd3ss3220->regmap,
> > +				HD3SS3220_REG_CN_STAT_CTRL,
> > +				data |
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
> > +			if (ret < 0)
> > +				goto error;
> > +		}
> > +		if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
> > +			ret = regmap_write(hd3ss3220->regmap,
> > +				HD3SS3220_REG_CN_STAT_CTRL,
> > +				data |
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
> > +			if (ret < 0)
> > +				goto error;
> > +		}
> > +
> > +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> NULL,
> > +					hd3ss3220_irq_handler,
> > +					IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT,
> > +					"hd3ss3220", &client->dev);
> > +		if (ret)
> > +			goto error;
> > +	}
> 
> I would move the above if (client->irq > 0) case to it's own function.

OK. Will fix this in V5.

Regards,
Biju

> > +	ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
> > +	if (ret < 0)
> > +		goto error;
> > +
> > +	dev_info(&client->dev, "probed revision=0x%x\n", ret);
> > +
> > +	return 0;
> > +error:
> > +	typec_unregister_port(hd3ss3220->port);
> > +	usb_role_switch_put(hd3ss3220->role_sw);
> > +
> > +	return ret;
> > +}
> > +
> > +static int hd3ss3220_remove(struct i2c_client *client) {
> > +	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> > +
> > +	typec_unregister_port(hd3ss3220->port);
> > +	usb_role_switch_put(hd3ss3220->role_sw);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id dev_ids[] = {
> > +	{ .compatible = "ti,hd3ss3220"},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, dev_ids);
> > +
> > +static struct i2c_driver hd3ss3220_driver = {
> > +	.driver = {
> > +		.name = "hd3ss3220",
> > +		.of_match_table = of_match_ptr(dev_ids),
> > +	},
> > +	.probe = hd3ss3220_probe,
> > +	.remove =  hd3ss3220_remove,
> > +};
> > +
> > +module_i2c_driver(hd3ss3220_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das@bp.renesas.com>");
> > +MODULE_DESCRIPTION("TI HD3SS3220 DRP Port Controller Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.7.4
> 
> thanks,
> 
> --
> heikki

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
@ 2019-04-23  2:27     ` Yoshihiro Shimoda
  0 siblings, 0 replies; 25+ messages in thread
From: Yoshihiro Shimoda @ 2019-04-23  2:27 UTC (permalink / raw)
  To: Biju Das
  Cc: Biju Das, Greg Kroah-Hartman, Heikki Krogerus, Simon Horman,
	Fabrizio Castro, Kees Cook, linux-usb, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, linux-renesas-soc,
	Felipe Balbi

Hi Biju-san,

Thank you for the patch!

> From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM
> 
> RZ/G2E cat874 board is capable of detecting cable connect and disconnect
> events. Add support for renesas_usb3 to receive connect and disconnect
> events and support dual-role switch using usb-role-switch framework.
> 
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> ---
>  V3-->V4
>    * No Change
>  V2-->V3
>    * Incorporated Shimoda-san's review comment
>      (https://patchwork.kernel.org/patch/10852507/)
>    * Used renesas,usb-role-switch property for differentiating USB
>      role switch associated with Type-C port controller driver.
>  V1-->V2
>    * Driver uses usb role clas for handling dual role switch and handling
>      connect/disconnect events instead of extcon.
<snip>

> +static void usb3_check_vbus(struct renesas_usb3 *usb3)
> +{
> +	if (usb3->workaround_for_vbus) {
> +		if (usb3->usb_role_switch_property) {
> +			if (usb3->connection_state == USB_ROLE_DEVICE) {
> +				usb3_mode_config(usb3, false, false);

I should have pointed it out the previous version though,
why does this usb3_mode_config() calling need?
My guess is:
 - renesas_usb3_start() calls renesas_usb3_init_controller().
 -- renesas_usb3_init_controller() calls usb3_check_id() and then usb_check_vbus().
 --- usb_check_id() calls usb3_mode_config(usb3, true, true) and then the HW acts as host mode.
 ----> So, you'd like the HW to acts as peripheral mode when the connection_state is USB_ROLE_DEVICE,
       you added that the usb3_check_vbus() calls usb3_mode_config(usb3, false, false).

Is my guess correct? If so, I'd like to add such code into usb3_check_id() like below:

	if ((usb3->extcon_host && !usb3->forced_b_device) ||
	    (usb3->usb_role_switch_property &&
	     usb3->connection_state == USB_ROLE_HOST))
		usb3_mode_config(usb3, true, true);
	else
		usb3_mode_config(usb3, false, false);

What do you think?

Best regards,
Yoshihiro Shimoda


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [V4,4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
@ 2019-04-23  2:27     ` Yoshihiro Shimoda
  0 siblings, 0 replies; 25+ messages in thread
From: Yoshihiro Shimoda @ 2019-04-23  2:27 UTC (permalink / raw)
  To: Biju Das
  Cc: Greg Kroah-Hartman, Heikki Krogerus, Simon Horman,
	Fabrizio Castro, Kees Cook, linux-usb, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, linux-renesas-soc,
	Felipe Balbi

Hi Biju-san,

Thank you for the patch!

> From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM
> 
> RZ/G2E cat874 board is capable of detecting cable connect and disconnect
> events. Add support for renesas_usb3 to receive connect and disconnect
> events and support dual-role switch using usb-role-switch framework.
> 
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> ---
>  V3-->V4
>    * No Change
>  V2-->V3
>    * Incorporated Shimoda-san's review comment
>      (https://patchwork.kernel.org/patch/10852507/)
>    * Used renesas,usb-role-switch property for differentiating USB
>      role switch associated with Type-C port controller driver.
>  V1-->V2
>    * Driver uses usb role clas for handling dual role switch and handling
>      connect/disconnect events instead of extcon.
<snip>

> +static void usb3_check_vbus(struct renesas_usb3 *usb3)
> +{
> +	if (usb3->workaround_for_vbus) {
> +		if (usb3->usb_role_switch_property) {
> +			if (usb3->connection_state == USB_ROLE_DEVICE) {
> +				usb3_mode_config(usb3, false, false);

I should have pointed it out the previous version though,
why does this usb3_mode_config() calling need?
My guess is:
 - renesas_usb3_start() calls renesas_usb3_init_controller().
 -- renesas_usb3_init_controller() calls usb3_check_id() and then usb_check_vbus().
 --- usb_check_id() calls usb3_mode_config(usb3, true, true) and then the HW acts as host mode.
 ----> So, you'd like the HW to acts as peripheral mode when the connection_state is USB_ROLE_DEVICE,
       you added that the usb3_check_vbus() calls usb3_mode_config(usb3, false, false).

Is my guess correct? If so, I'd like to add such code into usb3_check_id() like below:

	if ((usb3->extcon_host && !usb3->forced_b_device) ||
	    (usb3->usb_role_switch_property &&
	     usb3->connection_state == USB_ROLE_HOST))
		usb3_mode_config(usb3, true, true);
	else
		usb3_mode_config(usb3, false, false);

What do you think?

Best regards,
Yoshihiro Shimoda

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
@ 2019-04-23  9:07       ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-23  9:07 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Greg Kroah-Hartman, Heikki Krogerus, Simon Horman,
	Fabrizio Castro, Kees Cook, linux-usb, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, linux-renesas-soc,
	Felipe Balbi

Hi Shimoda-San,

Thanks for the feedback.

> Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use
> usb_role_switch framework
> 
> Hi Biju-san,
> 
> Thank you for the patch!
> 
> > From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM
> >
> > RZ/G2E cat874 board is capable of detecting cable connect and
> > disconnect events. Add support for renesas_usb3 to receive connect and
> > disconnect events and support dual-role switch using usb-role-switch
> framework.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > ---
> >  V3-->V4
> >    * No Change
> >  V2-->V3
> >    * Incorporated Shimoda-san's review comment
> >      (https://patchwork.kernel.org/patch/10852507/)
> >    * Used renesas,usb-role-switch property for differentiating USB
> >      role switch associated with Type-C port controller driver.
> >  V1-->V2
> >    * Driver uses usb role clas for handling dual role switch and handling
> >      connect/disconnect events instead of extcon.
> <snip>
> 
> > +static void usb3_check_vbus(struct renesas_usb3 *usb3) {
> > +	if (usb3->workaround_for_vbus) {
> > +		if (usb3->usb_role_switch_property) {
> > +			if (usb3->connection_state == USB_ROLE_DEVICE) {
> > +				usb3_mode_config(usb3, false, false);
> 
> I should have pointed it out the previous version though, why does this
> usb3_mode_config() calling need?
> My guess is:
>  - renesas_usb3_start() calls renesas_usb3_init_controller().
>  -- renesas_usb3_init_controller() calls usb3_check_id() and then
> usb_check_vbus().
>  --- usb_check_id() calls usb3_mode_config(usb3, true, true) and then the
> HW acts as host mode.
>  ----> So, you'd like the HW to acts as peripheral mode when the
> connection_state is USB_ROLE_DEVICE,
>        you added that the usb3_check_vbus() calls usb3_mode_config(usb3,
> false, false).
> 
> Is my guess correct? If so, I'd like to add such code into usb3_check_id() like
> below:

Yes, it is almost correct. The scenario I am trying is

[1] USB type C  cable connected to a Host Machine(TI chip identifies as Device connection. But we haven't installed Gadget module for Device operation)

[2] After that trying to install gadget module. In this case,  it calls usb_check_id() as mentioned above
and configure it as Host mode.

> 	if ((usb3->extcon_host && !usb3->forced_b_device) ||
> 	    (usb3->usb_role_switch_property &&
> 	     usb3->connection_state == USB_ROLE_HOST))
> 		usb3_mode_config(usb3, true, true);
> 	else
> 		usb3_mode_config(usb3, false, false);
> 
> What do you think?

Since as per [1], usb3->extcon_host=1 and !usb3->forced_b_device ==1  ,  The above code always enter into Host Mode configuration.

To make it work,  I need to update  " usb3->forced_b_device" based on connection_state from TI chip. So the new code look like

1) There is no change in usb_check_id() call.

if (usb3->extcon_host && !usb3->forced_b_device)
    usb3_mode_config(usb3, true, true);
   else
    usb3_mode_config(usb3, false, false);

2) Update "usb3->forced_b_device" variable  based on connection_state.

@@ -2370,6 +2376,7 @@ static void handle_ext_role_switch_states(struct device *dev,
                usb3_vbus_out(usb3, false);
                break;
        case USB_ROLE_DEVICE:
+               usb3->forced_b_device = true;
                if (usb3->connection_state == USB_ROLE_NONE) {
                        usb3->connection_state = USB_ROLE_DEVICE;
                        usb3_set_mode(usb3, false);
@@ -2384,6 +2391,7 @@ static void handle_ext_role_switch_states(struct device *dev,
                usb3_vbus_out(usb3, false);
                break;
        case USB_ROLE_HOST:
+               usb3->forced_b_device = false;

Can you please confirm are you ok with this changes? Or do you prefer the previous one?

Note:- 
 I have done only sanity testing with this changes.

Regards,
Biju








^ permalink raw reply	[flat|nested] 25+ messages in thread

* [V4,4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
@ 2019-04-23  9:07       ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-23  9:07 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Greg Kroah-Hartman, Heikki Krogerus, Simon Horman,
	Fabrizio Castro, Kees Cook, linux-usb, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, linux-renesas-soc,
	Felipe Balbi

Hi Shimoda-San,

Thanks for the feedback.

> Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use
> usb_role_switch framework
> 
> Hi Biju-san,
> 
> Thank you for the patch!
> 
> > From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM
> >
> > RZ/G2E cat874 board is capable of detecting cable connect and
> > disconnect events. Add support for renesas_usb3 to receive connect and
> > disconnect events and support dual-role switch using usb-role-switch
> framework.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > ---
> >  V3-->V4
> >    * No Change
> >  V2-->V3
> >    * Incorporated Shimoda-san's review comment
> >      (https://patchwork.kernel.org/patch/10852507/)
> >    * Used renesas,usb-role-switch property for differentiating USB
> >      role switch associated with Type-C port controller driver.
> >  V1-->V2
> >    * Driver uses usb role clas for handling dual role switch and handling
> >      connect/disconnect events instead of extcon.
> <snip>
> 
> > +static void usb3_check_vbus(struct renesas_usb3 *usb3) {
> > +	if (usb3->workaround_for_vbus) {
> > +		if (usb3->usb_role_switch_property) {
> > +			if (usb3->connection_state == USB_ROLE_DEVICE) {
> > +				usb3_mode_config(usb3, false, false);
> 
> I should have pointed it out the previous version though, why does this
> usb3_mode_config() calling need?
> My guess is:
>  - renesas_usb3_start() calls renesas_usb3_init_controller().
>  -- renesas_usb3_init_controller() calls usb3_check_id() and then
> usb_check_vbus().
>  --- usb_check_id() calls usb3_mode_config(usb3, true, true) and then the
> HW acts as host mode.
>  ----> So, you'd like the HW to acts as peripheral mode when the
> connection_state is USB_ROLE_DEVICE,
>        you added that the usb3_check_vbus() calls usb3_mode_config(usb3,
> false, false).
> 
> Is my guess correct? If so, I'd like to add such code into usb3_check_id() like
> below:

Yes, it is almost correct. The scenario I am trying is

[1] USB type C  cable connected to a Host Machine(TI chip identifies as Device connection. But we haven't installed Gadget module for Device operation)

[2] After that trying to install gadget module. In this case,  it calls usb_check_id() as mentioned above
and configure it as Host mode.

> 	if ((usb3->extcon_host && !usb3->forced_b_device) ||
> 	    (usb3->usb_role_switch_property &&
> 	     usb3->connection_state == USB_ROLE_HOST))
> 		usb3_mode_config(usb3, true, true);
> 	else
> 		usb3_mode_config(usb3, false, false);
> 
> What do you think?

Since as per [1], usb3->extcon_host=1 and !usb3->forced_b_device ==1  ,  The above code always enter into Host Mode configuration.

To make it work,  I need to update  " usb3->forced_b_device" based on connection_state from TI chip. So the new code look like

1) There is no change in usb_check_id() call.

if (usb3->extcon_host && !usb3->forced_b_device)
    usb3_mode_config(usb3, true, true);
   else
    usb3_mode_config(usb3, false, false);

2) Update "usb3->forced_b_device" variable  based on connection_state.

@@ -2370,6 +2376,7 @@ static void handle_ext_role_switch_states(struct device *dev,
                usb3_vbus_out(usb3, false);
                break;
        case USB_ROLE_DEVICE:
+               usb3->forced_b_device = true;
                if (usb3->connection_state == USB_ROLE_NONE) {
                        usb3->connection_state = USB_ROLE_DEVICE;
                        usb3_set_mode(usb3, false);
@@ -2384,6 +2391,7 @@ static void handle_ext_role_switch_states(struct device *dev,
                usb3_vbus_out(usb3, false);
                break;
        case USB_ROLE_HOST:
+               usb3->forced_b_device = false;

Can you please confirm are you ok with this changes? Or do you prefer the previous one?

Note:- 
 I have done only sanity testing with this changes.

Regards,
Biju

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
@ 2019-04-23 11:00         ` Yoshihiro Shimoda
  0 siblings, 0 replies; 25+ messages in thread
From: Yoshihiro Shimoda @ 2019-04-23 11:00 UTC (permalink / raw)
  To: Biju Das
  Cc: Greg Kroah-Hartman, Heikki Krogerus, Simon Horman,
	Fabrizio Castro, Kees Cook, linux-usb, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, linux-renesas-soc,
	Felipe Balbi

Hi Biju-san,

> From: Biju Das, Sent: Tuesday, April 23, 2019 6:08 PM
> 
> Hi Shimoda-San,
> 
> Thanks for the feedback.
> 
> > Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use
> > usb_role_switch framework
> >
> > Hi Biju-san,
> >
> > Thank you for the patch!
> >
> > > From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM
> > >
> > > RZ/G2E cat874 board is capable of detecting cable connect and
> > > disconnect events. Add support for renesas_usb3 to receive connect and
> > > disconnect events and support dual-role switch using usb-role-switch
> > framework.
> > >
> > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > > ---
> > >  V3-->V4
> > >    * No Change
> > >  V2-->V3
> > >    * Incorporated Shimoda-san's review comment
> > >      (https://patchwork.kernel.org/patch/10852507/)
> > >    * Used renesas,usb-role-switch property for differentiating USB
> > >      role switch associated with Type-C port controller driver.
> > >  V1-->V2
> > >    * Driver uses usb role clas for handling dual role switch and handling
> > >      connect/disconnect events instead of extcon.
> > <snip>
> >
> > > +static void usb3_check_vbus(struct renesas_usb3 *usb3) {
> > > +	if (usb3->workaround_for_vbus) {
> > > +		if (usb3->usb_role_switch_property) {
> > > +			if (usb3->connection_state == USB_ROLE_DEVICE) {
> > > +				usb3_mode_config(usb3, false, false);
> >
> > I should have pointed it out the previous version though, why does this
> > usb3_mode_config() calling need?
> > My guess is:
> >  - renesas_usb3_start() calls renesas_usb3_init_controller().
> >  -- renesas_usb3_init_controller() calls usb3_check_id() and then
> > usb_check_vbus().
> >  --- usb_check_id() calls usb3_mode_config(usb3, true, true) and then the
> > HW acts as host mode.
> >  ----> So, you'd like the HW to acts as peripheral mode when the
> > connection_state is USB_ROLE_DEVICE,
> >        you added that the usb3_check_vbus() calls usb3_mode_config(usb3,
> > false, false).
> >
> > Is my guess correct? If so, I'd like to add such code into usb3_check_id() like
> > below:
> 
> Yes, it is almost correct. The scenario I am trying is
> 
> [1] USB type C  cable connected to a Host Machine(TI chip identifies as Device connection. But we haven't installed Gadget
> module for Device operation)
> 
> [2] After that trying to install gadget module. In this case,  it calls usb_check_id() as mentioned above
> and configure it as Host mode.

Thank you for the explanation.

> > 	if ((usb3->extcon_host && !usb3->forced_b_device) ||
> > 	    (usb3->usb_role_switch_property &&
> > 	     usb3->connection_state == USB_ROLE_HOST))
> > 		usb3_mode_config(usb3, true, true);
> > 	else
> > 		usb3_mode_config(usb3, false, false);
> >
> > What do you think?
> 
> Since as per [1], usb3->extcon_host=1 and !usb3->forced_b_device ==1  ,  The above code always enter into Host Mode
> configuration.

Oops. Thank you for the pointed it out.

> To make it work,  I need to update  " usb3->forced_b_device" based on connection_state from TI chip. So the new code look
> like

Since the forced_b_device is related to debug purpose (controlled by debugfs), I don't want to use the value for type-c.

> 1) There is no change in usb_check_id() call.
> 
> if (usb3->extcon_host && !usb3->forced_b_device)
>     usb3_mode_config(usb3, true, true);
>    else
>     usb3_mode_config(usb3, false, false);
> 
> 2) Update "usb3->forced_b_device" variable  based on connection_state.
> 
> @@ -2370,6 +2376,7 @@ static void handle_ext_role_switch_states(struct device *dev,
>                 usb3_vbus_out(usb3, false);
>                 break;
>         case USB_ROLE_DEVICE:
> +               usb3->forced_b_device = true;
>                 if (usb3->connection_state == USB_ROLE_NONE) {
>                         usb3->connection_state = USB_ROLE_DEVICE;
>                         usb3_set_mode(usb3, false);
> @@ -2384,6 +2391,7 @@ static void handle_ext_role_switch_states(struct device *dev,
>                 usb3_vbus_out(usb3, false);
>                 break;
>         case USB_ROLE_HOST:
> +               usb3->forced_b_device = false;
> 
> Can you please confirm are you ok with this changes? Or do you prefer the previous one?

I'd like to change usb3_check_id() somehow.
How about the following conditions? In type-c environment,
since usb3->usb_role_switch_property is true, it should be OK for it.

	if ((!usb3->usb_role_switch_property &&
	     usb3->extcon_host && !usb3->forced_b_device) ||
	    (usb3->usb_role_switch_property &&
	     usb3->connection_state == USB_ROLE_HOST))
		usb3_mode_config(usb3, true, true);
	else
		usb3_mode_config(usb3, false, false);

Best regards,
Yoshihiro Shimoda

> Note:-
>  I have done only sanity testing with this changes.
> 
> Regards,
> Biju
> 
> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [V4,4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
@ 2019-04-23 11:00         ` Yoshihiro Shimoda
  0 siblings, 0 replies; 25+ messages in thread
From: Yoshihiro Shimoda @ 2019-04-23 11:00 UTC (permalink / raw)
  To: Biju Das
  Cc: Greg Kroah-Hartman, Heikki Krogerus, Simon Horman,
	Fabrizio Castro, Kees Cook, linux-usb, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, linux-renesas-soc,
	Felipe Balbi

Hi Biju-san,

> From: Biju Das, Sent: Tuesday, April 23, 2019 6:08 PM
> 
> Hi Shimoda-San,
> 
> Thanks for the feedback.
> 
> > Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use
> > usb_role_switch framework
> >
> > Hi Biju-san,
> >
> > Thank you for the patch!
> >
> > > From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM
> > >
> > > RZ/G2E cat874 board is capable of detecting cable connect and
> > > disconnect events. Add support for renesas_usb3 to receive connect and
> > > disconnect events and support dual-role switch using usb-role-switch
> > framework.
> > >
> > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > > ---
> > >  V3-->V4
> > >    * No Change
> > >  V2-->V3
> > >    * Incorporated Shimoda-san's review comment
> > >      (https://patchwork.kernel.org/patch/10852507/)
> > >    * Used renesas,usb-role-switch property for differentiating USB
> > >      role switch associated with Type-C port controller driver.
> > >  V1-->V2
> > >    * Driver uses usb role clas for handling dual role switch and handling
> > >      connect/disconnect events instead of extcon.
> > <snip>
> >
> > > +static void usb3_check_vbus(struct renesas_usb3 *usb3) {
> > > +	if (usb3->workaround_for_vbus) {
> > > +		if (usb3->usb_role_switch_property) {
> > > +			if (usb3->connection_state == USB_ROLE_DEVICE) {
> > > +				usb3_mode_config(usb3, false, false);
> >
> > I should have pointed it out the previous version though, why does this
> > usb3_mode_config() calling need?
> > My guess is:
> >  - renesas_usb3_start() calls renesas_usb3_init_controller().
> >  -- renesas_usb3_init_controller() calls usb3_check_id() and then
> > usb_check_vbus().
> >  --- usb_check_id() calls usb3_mode_config(usb3, true, true) and then the
> > HW acts as host mode.
> >  ----> So, you'd like the HW to acts as peripheral mode when the
> > connection_state is USB_ROLE_DEVICE,
> >        you added that the usb3_check_vbus() calls usb3_mode_config(usb3,
> > false, false).
> >
> > Is my guess correct? If so, I'd like to add such code into usb3_check_id() like
> > below:
> 
> Yes, it is almost correct. The scenario I am trying is
> 
> [1] USB type C  cable connected to a Host Machine(TI chip identifies as Device connection. But we haven't installed Gadget
> module for Device operation)
> 
> [2] After that trying to install gadget module. In this case,  it calls usb_check_id() as mentioned above
> and configure it as Host mode.

Thank you for the explanation.

> > 	if ((usb3->extcon_host && !usb3->forced_b_device) ||
> > 	    (usb3->usb_role_switch_property &&
> > 	     usb3->connection_state == USB_ROLE_HOST))
> > 		usb3_mode_config(usb3, true, true);
> > 	else
> > 		usb3_mode_config(usb3, false, false);
> >
> > What do you think?
> 
> Since as per [1], usb3->extcon_host=1 and !usb3->forced_b_device ==1  ,  The above code always enter into Host Mode
> configuration.

Oops. Thank you for the pointed it out.

> To make it work,  I need to update  " usb3->forced_b_device" based on connection_state from TI chip. So the new code look
> like

Since the forced_b_device is related to debug purpose (controlled by debugfs), I don't want to use the value for type-c.

> 1) There is no change in usb_check_id() call.
> 
> if (usb3->extcon_host && !usb3->forced_b_device)
>     usb3_mode_config(usb3, true, true);
>    else
>     usb3_mode_config(usb3, false, false);
> 
> 2) Update "usb3->forced_b_device" variable  based on connection_state.
> 
> @@ -2370,6 +2376,7 @@ static void handle_ext_role_switch_states(struct device *dev,
>                 usb3_vbus_out(usb3, false);
>                 break;
>         case USB_ROLE_DEVICE:
> +               usb3->forced_b_device = true;
>                 if (usb3->connection_state == USB_ROLE_NONE) {
>                         usb3->connection_state = USB_ROLE_DEVICE;
>                         usb3_set_mode(usb3, false);
> @@ -2384,6 +2391,7 @@ static void handle_ext_role_switch_states(struct device *dev,
>                 usb3_vbus_out(usb3, false);
>                 break;
>         case USB_ROLE_HOST:
> +               usb3->forced_b_device = false;
> 
> Can you please confirm are you ok with this changes? Or do you prefer the previous one?

I'd like to change usb3_check_id() somehow.
How about the following conditions? In type-c environment,
since usb3->usb_role_switch_property is true, it should be OK for it.

	if ((!usb3->usb_role_switch_property &&
	     usb3->extcon_host && !usb3->forced_b_device) ||
	    (usb3->usb_role_switch_property &&
	     usb3->connection_state == USB_ROLE_HOST))
		usb3_mode_config(usb3, true, true);
	else
		usb3_mode_config(usb3, false, false);

Best regards,
Yoshihiro Shimoda

> Note:-
>  I have done only sanity testing with this changes.
> 
> Regards,
> Biju
> 
> 
> 
> 
> 
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
@ 2019-04-23 15:06           ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-23 15:06 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Greg Kroah-Hartman, Heikki Krogerus, Simon Horman,
	Fabrizio Castro, Kees Cook, linux-usb, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, linux-renesas-soc,
	Felipe Balbi

Hi Shimoda-San,

Thanks for the feedback.

> Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use
> usb_role_switch framework
> 
> Hi Biju-san,
> 
> > From: Biju Das, Sent: Tuesday, April 23, 2019 6:08 PM
> >
> > Hi Shimoda-San,
> >
> > Thanks for the feedback.
> >
> > > Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use
> > > usb_role_switch framework
> > >
> > > Hi Biju-san,
> > >
> > > Thank you for the patch!
> > >
> > > > From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM
> > > >
> > > > RZ/G2E cat874 board is capable of detecting cable connect and
> > > > disconnect events. Add support for renesas_usb3 to receive connect
> > > > and disconnect events and support dual-role switch using
> > > > usb-role-switch
> > > framework.
> > > >
> > > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > > > ---
> > > >  V3-->V4
> > > >    * No Change
> > > >  V2-->V3
> > > >    * Incorporated Shimoda-san's review comment
> > > >      (https://patchwork.kernel.org/patch/10852507/)
> > > >    * Used renesas,usb-role-switch property for differentiating USB
> > > >      role switch associated with Type-C port controller driver.
> > > >  V1-->V2
> > > >    * Driver uses usb role clas for handling dual role switch and handling
> > > >      connect/disconnect events instead of extcon.
> > > <snip>
> > >
> > > > +static void usb3_check_vbus(struct renesas_usb3 *usb3) {
> > > > +	if (usb3->workaround_for_vbus) {
> > > > +		if (usb3->usb_role_switch_property) {
> > > > +			if (usb3->connection_state == USB_ROLE_DEVICE) {
> > > > +				usb3_mode_config(usb3, false, false);
> > >
> > > I should have pointed it out the previous version though, why does
> > > this
> > > usb3_mode_config() calling need?
> > > My guess is:
> > >  - renesas_usb3_start() calls renesas_usb3_init_controller().
> > >  -- renesas_usb3_init_controller() calls usb3_check_id() and then
> > > usb_check_vbus().
> > >  --- usb_check_id() calls usb3_mode_config(usb3, true, true) and
> > > then the HW acts as host mode.
> > >  ----> So, you'd like the HW to acts as peripheral mode when the
> > > connection_state is USB_ROLE_DEVICE,
> > >        you added that the usb3_check_vbus() calls
> > > usb3_mode_config(usb3, false, false).
> > >
> > > Is my guess correct? If so, I'd like to add such code into
> > > usb3_check_id() like
> > > below:
> >
> > Yes, it is almost correct. The scenario I am trying is
> >
> > [1] USB type C  cable connected to a Host Machine(TI chip identifies
> > as Device connection. But we haven't installed Gadget module for
> > Device operation)
> >
> > [2] After that trying to install gadget module. In this case,  it
> > calls usb_check_id() as mentioned above and configure it as Host mode.
> 
> Thank you for the explanation.
> 
> > > 	if ((usb3->extcon_host && !usb3->forced_b_device) ||
> > > 	    (usb3->usb_role_switch_property &&
> > > 	     usb3->connection_state == USB_ROLE_HOST))
> > > 		usb3_mode_config(usb3, true, true);
> > > 	else
> > > 		usb3_mode_config(usb3, false, false);
> > >
> > > What do you think?
> >
> > Since as per [1], usb3->extcon_host=1 and !usb3->forced_b_device ==1
> > ,  The above code always enter into Host Mode configuration.
> 
> Oops. Thank you for the pointed it out.
> 
> > To make it work,  I need to update  " usb3->forced_b_device" based on
> > connection_state from TI chip. So the new code look like
> 
> Since the forced_b_device is related to debug purpose (controlled by
> debugfs), I don't want to use the value for type-c.
> 
> > 1) There is no change in usb_check_id() call.
> >
> > if (usb3->extcon_host && !usb3->forced_b_device)
> >     usb3_mode_config(usb3, true, true);
> >    else
> >     usb3_mode_config(usb3, false, false);
> >
> > 2) Update "usb3->forced_b_device" variable  based on connection_state.
> >
> > @@ -2370,6 +2376,7 @@ static void handle_ext_role_switch_states(struct
> device *dev,
> >                 usb3_vbus_out(usb3, false);
> >                 break;
> >         case USB_ROLE_DEVICE:
> > +               usb3->forced_b_device = true;
> >                 if (usb3->connection_state == USB_ROLE_NONE) {
> >                         usb3->connection_state = USB_ROLE_DEVICE;
> >                         usb3_set_mode(usb3, false); @@ -2384,6 +2391,7
> > @@ static void handle_ext_role_switch_states(struct device *dev,
> >                 usb3_vbus_out(usb3, false);
> >                 break;
> >         case USB_ROLE_HOST:
> > +               usb3->forced_b_device = false;
> >
> > Can you please confirm are you ok with this changes? Or do you prefer the
> previous one?
> 
> I'd like to change usb3_check_id() somehow.
> How about the following conditions? In type-c environment, since usb3-
> >usb_role_switch_property is true, it should be OK for it.
> 
> 	if ((!usb3->usb_role_switch_property &&
> 	     usb3->extcon_host && !usb3->forced_b_device) ||
> 	    (usb3->usb_role_switch_property &&
> 	     usb3->connection_state == USB_ROLE_HOST))
> 		usb3_mode_config(usb3, true, true);
> 	else
> 		usb3_mode_config(usb3, false, false);
> 

OK. I will send V5 with this changes.

Regards,
Biju

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [V4,4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework
@ 2019-04-23 15:06           ` Biju Das
  0 siblings, 0 replies; 25+ messages in thread
From: Biju Das @ 2019-04-23 15:06 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Greg Kroah-Hartman, Heikki Krogerus, Simon Horman,
	Fabrizio Castro, Kees Cook, linux-usb, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, linux-renesas-soc,
	Felipe Balbi

Hi Shimoda-San,

Thanks for the feedback.

> Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use
> usb_role_switch framework
> 
> Hi Biju-san,
> 
> > From: Biju Das, Sent: Tuesday, April 23, 2019 6:08 PM
> >
> > Hi Shimoda-San,
> >
> > Thanks for the feedback.
> >
> > > Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use
> > > usb_role_switch framework
> > >
> > > Hi Biju-san,
> > >
> > > Thank you for the patch!
> > >
> > > > From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM
> > > >
> > > > RZ/G2E cat874 board is capable of detecting cable connect and
> > > > disconnect events. Add support for renesas_usb3 to receive connect
> > > > and disconnect events and support dual-role switch using
> > > > usb-role-switch
> > > framework.
> > > >
> > > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > > > ---
> > > >  V3-->V4
> > > >    * No Change
> > > >  V2-->V3
> > > >    * Incorporated Shimoda-san's review comment
> > > >      (https://patchwork.kernel.org/patch/10852507/)
> > > >    * Used renesas,usb-role-switch property for differentiating USB
> > > >      role switch associated with Type-C port controller driver.
> > > >  V1-->V2
> > > >    * Driver uses usb role clas for handling dual role switch and handling
> > > >      connect/disconnect events instead of extcon.
> > > <snip>
> > >
> > > > +static void usb3_check_vbus(struct renesas_usb3 *usb3) {
> > > > +	if (usb3->workaround_for_vbus) {
> > > > +		if (usb3->usb_role_switch_property) {
> > > > +			if (usb3->connection_state == USB_ROLE_DEVICE) {
> > > > +				usb3_mode_config(usb3, false, false);
> > >
> > > I should have pointed it out the previous version though, why does
> > > this
> > > usb3_mode_config() calling need?
> > > My guess is:
> > >  - renesas_usb3_start() calls renesas_usb3_init_controller().
> > >  -- renesas_usb3_init_controller() calls usb3_check_id() and then
> > > usb_check_vbus().
> > >  --- usb_check_id() calls usb3_mode_config(usb3, true, true) and
> > > then the HW acts as host mode.
> > >  ----> So, you'd like the HW to acts as peripheral mode when the
> > > connection_state is USB_ROLE_DEVICE,
> > >        you added that the usb3_check_vbus() calls
> > > usb3_mode_config(usb3, false, false).
> > >
> > > Is my guess correct? If so, I'd like to add such code into
> > > usb3_check_id() like
> > > below:
> >
> > Yes, it is almost correct. The scenario I am trying is
> >
> > [1] USB type C  cable connected to a Host Machine(TI chip identifies
> > as Device connection. But we haven't installed Gadget module for
> > Device operation)
> >
> > [2] After that trying to install gadget module. In this case,  it
> > calls usb_check_id() as mentioned above and configure it as Host mode.
> 
> Thank you for the explanation.
> 
> > > 	if ((usb3->extcon_host && !usb3->forced_b_device) ||
> > > 	    (usb3->usb_role_switch_property &&
> > > 	     usb3->connection_state == USB_ROLE_HOST))
> > > 		usb3_mode_config(usb3, true, true);
> > > 	else
> > > 		usb3_mode_config(usb3, false, false);
> > >
> > > What do you think?
> >
> > Since as per [1], usb3->extcon_host=1 and !usb3->forced_b_device ==1
> > ,  The above code always enter into Host Mode configuration.
> 
> Oops. Thank you for the pointed it out.
> 
> > To make it work,  I need to update  " usb3->forced_b_device" based on
> > connection_state from TI chip. So the new code look like
> 
> Since the forced_b_device is related to debug purpose (controlled by
> debugfs), I don't want to use the value for type-c.
> 
> > 1) There is no change in usb_check_id() call.
> >
> > if (usb3->extcon_host && !usb3->forced_b_device)
> >     usb3_mode_config(usb3, true, true);
> >    else
> >     usb3_mode_config(usb3, false, false);
> >
> > 2) Update "usb3->forced_b_device" variable  based on connection_state.
> >
> > @@ -2370,6 +2376,7 @@ static void handle_ext_role_switch_states(struct
> device *dev,
> >                 usb3_vbus_out(usb3, false);
> >                 break;
> >         case USB_ROLE_DEVICE:
> > +               usb3->forced_b_device = true;
> >                 if (usb3->connection_state == USB_ROLE_NONE) {
> >                         usb3->connection_state = USB_ROLE_DEVICE;
> >                         usb3_set_mode(usb3, false); @@ -2384,6 +2391,7
> > @@ static void handle_ext_role_switch_states(struct device *dev,
> >                 usb3_vbus_out(usb3, false);
> >                 break;
> >         case USB_ROLE_HOST:
> > +               usb3->forced_b_device = false;
> >
> > Can you please confirm are you ok with this changes? Or do you prefer the
> previous one?
> 
> I'd like to change usb3_check_id() somehow.
> How about the following conditions? In type-c environment, since usb3-
> >usb_role_switch_property is true, it should be OK for it.
> 
> 	if ((!usb3->usb_role_switch_property &&
> 	     usb3->extcon_host && !usb3->forced_b_device) ||
> 	    (usb3->usb_role_switch_property &&
> 	     usb3->connection_state == USB_ROLE_HOST))
> 		usb3_mode_config(usb3, true, true);
> 	else
> 		usb3_mode_config(usb3, false, false);
> 

OK. I will send V5 with this changes.

Regards,
Biju

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2019-04-23 15:06 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-16  9:38 [PATCH V4 0/7] Add USB3.0 and TI HD3SS3220 driver support Biju Das
2019-04-16  9:38 ` [PATCH V4 1/7] dt-bindings: usb: hd3ss3220 device tree binding document Biju Das
2019-04-16  9:38   ` [V4,1/7] " Biju Das
2019-04-16  9:38 ` [PATCH V4 2/7] dt-bindings: usb: renesas_usb3: Add renesas,usb-role-switch property Biju Das
2019-04-16  9:38   ` [V4,2/7] " Biju Das
2019-04-16  9:38 ` [PATCH V4 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller Biju Das
2019-04-16  9:38   ` [V4,3/7] " Biju Das
2019-04-17  9:57   ` [PATCH V4 3/7] " Heikki Krogerus
2019-04-17  9:57     ` [V4,3/7] " Heikki Krogerus
2019-04-17 11:07     ` [PATCH V4 3/7] " Biju Das
2019-04-17 11:07       ` [V4,3/7] " Biju Das
2019-04-16  9:38 ` [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework Biju Das
2019-04-16  9:38   ` [V4,4/7] " Biju Das
2019-04-23  2:27   ` [PATCH V4 4/7] " Yoshihiro Shimoda
2019-04-23  2:27     ` [V4,4/7] " Yoshihiro Shimoda
2019-04-23  9:07     ` [PATCH V4 4/7] " Biju Das
2019-04-23  9:07       ` [V4,4/7] " Biju Das
2019-04-23 11:00       ` [PATCH V4 4/7] " Yoshihiro Shimoda
2019-04-23 11:00         ` [V4,4/7] " Yoshihiro Shimoda
2019-04-23 15:06         ` [PATCH V4 4/7] " Biju Das
2019-04-23 15:06           ` [V4,4/7] " Biju Das
2019-04-16  9:38 ` [PATCH V4 5/7] arm64: defconfig: enable TYPEC_HD3SS3220 config option Biju Das
2019-04-16  9:38   ` [V4,5/7] " Biju Das
2019-04-16  9:38 ` [PATCH V4 6/7] arm64: dts: renesas: r8a774c0-cat874: Enable USB3.0 host/peripheral device node Biju Das
2019-04-16  9:38 ` [PATCH V4 7/7] arm64: dts: renesas: r8a774c0-cat874: Enable usb role switch support Biju Das

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.