[PATCH v6 0/7] Add USB3.0 and TI HD3SS3220 driver support

[PATCH v6 0/7] Add USB3.0 and TI HD3SS3220 driver support

[Biju Das]

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-20190514 branch.
V5-->V6
  * Updated bindings patch by using usb-role-switch property
  * Used graph api's to get role switch supported by the usb-c-connector
V4-->V5
  * Incorporated Heikki's review comment.
    (https://patchwork.kernel.org/patch/10902531/)
  * Patch 1 is a dependency patch to make the compilation successful.
    (https://patchwork.kernel.org/patch/10909971/)
  * Patch 2 is also dependency patch to make the compilation successful.
    Rebased on top of Patch1 and fixed Heikki's review comments
    (https://patchwork.kernel.org/patch/10882555/)
  * Incorporated Shimoda-San's review comment.
    (https://patchwork.kernel.org/patch/10902535/)
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: Document usb role switch support
  usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller
  usb: gadget: udc: renesas_usb3: Add dual role switch support
  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       |  26 ++
 .../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              | 121 +++++++++-
 drivers/usb/typec/Kconfig                          |  10 +
 drivers/usb/typec/Makefile                         |   1 +
 drivers/usb/typec/hd3ss3220.c                      | 263 +++++++++++++++++++++
 8 files changed, 509 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/ti,hd3ss3220.txt
 create mode 100644 drivers/usb/typec/hd3ss3220.c

-- 
2.7.4

[PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding document

[Biju Das]

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>
---
V5-->V6
  * No change.
V4-->V5
  * No Change.
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

[PATCH v6 2/7] dt-bindings: usb: renesas_usb3: Document usb role switch support

[Biju Das]

Update the DT bindings documentation to support usb role switch
for USB Type-C connector using USB role switch class framework.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 V5-->V6
  * Updated description
  * Added usb-role-switch-property
 V4-->V5
  * No Change
 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       | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
index 35039e7..ea6c63c 100644
--- a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
+++ b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
@@ -22,6 +22,11 @@ Required properties:
 Optional properties:
   - phys: phandle + phy specifier pair
   - phy-names: must be "usb"
+  - usb-role-switch: support role switch. see usb/generic.txt
+
+Sub-nodes:
+The port would be added as a subnode if the "usb-role-switch" property is used.
+	see graph.txt
 
 Example of R-Car H3 ES1.x:
 	usb3_peri0: usb@ee020000 {
@@ -39,3 +44,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>;
+		usb-role-switch;
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			usb3peri_role_switch: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&hd3ss3220_ep>;
+			};
+		};
+	};
-- 
2.7.4

[PATCH v6 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller

[Biju Das]

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>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 V5-->V6
   * No change
   Note: This patch depend on [1]
   [1]: [v5,4/6] usb: roles: add API to get usb_role_switch by node
   https://patchwork.kernel.org/patch/10942499/
 V4-->V5
   * Incorporated Heikki's review comment
     (https://patchwork.kernel.org/patch/10902531/)
   * Added Heikki's Reviewed-by tag
 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..7ef7e85
--- /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;
+		break;
+	}
+
+	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);
+	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 (client->irq > 0) {
+		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

[PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role switch support

[Biju Das]

The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220 usb
type-c drp port controller. This patch adds dual role switch support for
the type-c connector using the usb role switch class framework.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 V5-->V6
   * Added graph api's to find the role supported by the connector.
 V4-->V5
   * Incorporated Shimoda-san's review comment
    (https://patchwork.kernel.org/patch/10902537/)
 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 | 121 ++++++++++++++++++++++++++++++++--
 1 file changed, 114 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
index 7dc2485..1d41998 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -24,6 +24,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/of.h>
+#include <linux/of_graph.h>
 #include <linux/usb/role.h>
 
 /* register definitions */
@@ -351,6 +352,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 +362,7 @@ struct renesas_usb3 {
 	bool extcon_usb;		/* check vbus and set EXTCON_USB */
 	bool forced_b_device;
 	bool start_to_connect;
+	bool dual_role_sw;
 };
 
 #define gadget_to_renesas_usb3(_gadget)	\
@@ -699,8 +703,10 @@ 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->dual_role_sw || 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);
@@ -716,7 +722,8 @@ static void usb3_check_id(struct renesas_usb3 *usb3)
 {
 	usb3->extcon_host = usb3_is_a_device(usb3);
 
-	if (usb3->extcon_host && !usb3->forced_b_device)
+	if ((!usb3->dual_role_sw && usb3->extcon_host && !usb3->forced_b_device)
+	    || usb3->connection_state == USB_ROLE_HOST)
 		usb3_mode_config(usb3, true, true);
 	else
 		usb3_mode_config(usb3, false, false);
@@ -2343,14 +2350,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 +2419,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->dual_role_sw)
+		handle_ext_role_switch_states(dev, role);
+	else
+		handle_role_switch_states(dev, role);
+
 	pm_runtime_put(dev);
 
 	return 0;
@@ -2650,12 +2722,41 @@ 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,
 };
 
+static bool is_usb_dual_role_switch(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *parent;
+	struct device_node *child;
+	bool ret = false;
+	const char *role_type = NULL;
+
+	child = of_graph_get_endpoint_by_regs(np, -1, -1);
+	if (!child)
+		return ret;
+
+	parent = of_graph_get_remote_port_parent(child);
+	of_node_put(child);
+	child = of_get_child_by_name(parent, "connector");
+	of_node_put(parent);
+	if (!child)
+		return ret;
+
+	if (of_device_is_compatible(child, "usb-c-connector")) {
+		of_property_read_string(child, "data-role", &role_type);
+		if (role_type && (!strncmp(role_type, "dual", strlen("dual"))))
+			ret = true;
+	}
+
+	of_node_put(child);
+	return ret;
+}
+
 static int renesas_usb3_probe(struct platform_device *pdev)
 {
 	struct renesas_usb3 *usb3;
@@ -2741,6 +2842,12 @@ static int renesas_usb3_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_dev_create;
 
+	if (device_property_read_bool(&pdev->dev, "usb-role-switch") &&
+	    is_usb_dual_role_switch(&pdev->dev)) {
+		usb3->dual_role_sw = 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

[PATCH v6 5/7] arm64: defconfig: enable TYPEC_HD3SS3220 config option

[Biju Das]

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>
---
V5-->V6
  * No change
V4-->V5
  * No change
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 8871cf7..9dc71a7 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -576,6 +576,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

[PATCH v6 6/7] arm64: dts: renesas: r8a774c0-cat874: Enable USB3.0 host/peripheral device node

[Biju Das]

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

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
V5-->V6
  * No change
V4-->V5
  * No change
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

[PATCH v6 7/7] arm64: dts: renesas: r8a774c0-cat874: Enable usb role switch support

[Biju Das]

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>
---
V5-->V6
  * No change
V4-->V5
  * No change
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..124ed58 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";
+	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

Re: [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding document

[Sergei Shtylyov]

Hello!

On 05/15/2019 03:09 PM, Biju Das wrote:

> 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>
> ---
> V5-->V6
>   * No change.
> V4-->V5
>   * No Change.
> 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.

   This depends on an interrupt controller used. I'd just said "an interrupt
specifier", w/o further details.

> +
> +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
> +
[...]

MBR, Sergei

Re: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role switch support

[Chunfeng Yun]

On Wed, 2019-05-15 at 13:09 +0100, Biju Das wrote:
> The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220 usb
> type-c drp port controller. This patch adds dual role switch support for
> the type-c connector using the usb role switch class framework.
> 
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> ---
>  V5-->V6
>    * Added graph api's to find the role supported by the connector.
>  V4-->V5
>    * Incorporated Shimoda-san's review comment
>     (https://patchwork.kernel.org/patch/10902537/)
>  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 | 121 ++++++++++++++++++++++++++++++++--
>  1 file changed, 114 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
> index 7dc2485..1d41998 100644
> --- a/drivers/usb/gadget/udc/renesas_usb3.c
> +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> @@ -24,6 +24,7 @@
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/of.h>
> +#include <linux/of_graph.h>
>  #include <linux/usb/role.h>
>  
>  /* register definitions */
> @@ -351,6 +352,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 +362,7 @@ struct renesas_usb3 {
>  	bool extcon_usb;		/* check vbus and set EXTCON_USB */
>  	bool forced_b_device;
>  	bool start_to_connect;
> +	bool dual_role_sw;
>  };
>  
>  #define gadget_to_renesas_usb3(_gadget)	\
> @@ -699,8 +703,10 @@ 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->dual_role_sw || 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);
> @@ -716,7 +722,8 @@ static void usb3_check_id(struct renesas_usb3 *usb3)
>  {
>  	usb3->extcon_host = usb3_is_a_device(usb3);
>  
> -	if (usb3->extcon_host && !usb3->forced_b_device)
> +	if ((!usb3->dual_role_sw && usb3->extcon_host && !usb3->forced_b_device)
> +	    || usb3->connection_state == USB_ROLE_HOST)
>  		usb3_mode_config(usb3, true, true);
>  	else
>  		usb3_mode_config(usb3, false, false);
> @@ -2343,14 +2350,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 +2419,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->dual_role_sw)
> +		handle_ext_role_switch_states(dev, role);
> +	else
> +		handle_role_switch_states(dev, role);
> +
>  	pm_runtime_put(dev);
>  
>  	return 0;
> @@ -2650,12 +2722,41 @@ 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,
>  };
>  
> +static bool is_usb_dual_role_switch(struct device *dev)
To me, it's not good idea to pay an attention to specific consumer of
the role switch, assume any device could assign role if it get this USB
role switch, not only type-c connector

> +{
> +	struct device_node *np = dev->of_node;
> +	struct device_node *parent;
> +	struct device_node *child;
> +	bool ret = false;
> +	const char *role_type = NULL;
> +
> +	child = of_graph_get_endpoint_by_regs(np, -1, -1);
> +	if (!child)
> +		return ret;
> +
> +	parent = of_graph_get_remote_port_parent(child);
> +	of_node_put(child);
> +	child = of_get_child_by_name(parent, "connector");
> +	of_node_put(parent);
> +	if (!child)
> +		return ret;
> +
> +	if (of_device_is_compatible(child, "usb-c-connector")) {
> +		of_property_read_string(child, "data-role", &role_type);
> +		if (role_type && (!strncmp(role_type, "dual", strlen("dual"))))
> +			ret = true;
> +	}
> +
> +	of_node_put(child);
> +	return ret;
> +}
> +
>  static int renesas_usb3_probe(struct platform_device *pdev)
>  {
>  	struct renesas_usb3 *usb3;
> @@ -2741,6 +2842,12 @@ static int renesas_usb3_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_dev_create;
>  
> +	if (device_property_read_bool(&pdev->dev, "usb-role-switch") &&
> +	    is_usb_dual_role_switch(&pdev->dev)) {
> +		usb3->dual_role_sw = 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);

RE: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role switch support

[Biju Das]

+ Rob

Hi Chunfeng Yun,

Thanks for the feedback.

> Subject: Re: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role
> switch support
> 
> On Wed, 2019-05-15 at 13:09 +0100, Biju Das wrote:
> > The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220
> > usb type-c drp port controller. This patch adds dual role switch
> > support for the type-c connector using the usb role switch class framework.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > ---
> >  V5-->V6
> >    * Added graph api's to find the role supported by the connector.
> >  V4-->V5
> >    * Incorporated Shimoda-san's review comment
> >     (https://patchwork.kernel.org/patch/10902537/)
> >  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 | 121
> > ++++++++++++++++++++++++++++++++--
> >  1 file changed, 114 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c
> > b/drivers/usb/gadget/udc/renesas_usb3.c
> > index 7dc2485..1d41998 100644
> > --- a/drivers/usb/gadget/udc/renesas_usb3.c
> > +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/usb/ch9.h>
> >  #include <linux/usb/gadget.h>
> >  #include <linux/usb/of.h>
> > +#include <linux/of_graph.h>
> >  #include <linux/usb/role.h>
> >
> >  /* register definitions */
> > @@ -351,6 +352,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 +362,7 @@ struct renesas_usb3 {
> >  	bool extcon_usb;		/* check vbus and set EXTCON_USB
> */
> >  	bool forced_b_device;
> >  	bool start_to_connect;
> > +	bool dual_role_sw;
> >  };
> >
> >  #define gadget_to_renesas_usb3(_gadget)	\
> > @@ -699,8 +703,10 @@ 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->dual_role_sw || 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);
> > @@ -716,7 +722,8 @@ static void usb3_check_id(struct renesas_usb3
> > *usb3)  {
> >  	usb3->extcon_host = usb3_is_a_device(usb3);
> >
> > -	if (usb3->extcon_host && !usb3->forced_b_device)
> > +	if ((!usb3->dual_role_sw && usb3->extcon_host && !usb3-
> >forced_b_device)
> > +	    || usb3->connection_state == USB_ROLE_HOST)
> >  		usb3_mode_config(usb3, true, true);
> >  	else
> >  		usb3_mode_config(usb3, false, false); @@ -2343,14 +2350,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 +2419,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->dual_role_sw)
> > +		handle_ext_role_switch_states(dev, role);
> > +	else
> > +		handle_role_switch_states(dev, role);
> > +
> >  	pm_runtime_put(dev);
> >
> >  	return 0;
> > @@ -2650,12 +2722,41 @@ 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,
> >  };
> >
> > +static bool is_usb_dual_role_switch(struct device *dev)
> To me, it's not good idea to pay an attention to specific consumer of the role
> switch, assume any device could assign role if it get this USB role switch, not
> only type-c connector

Yes, I agree. 

I have previously posted a patch based on this [1].
[1]. https://patchwork.kernel.org/patch/10852505/

Then on the binding patch, Rob suggested   to walk the graph to the connector
and determine if dual role is supported by the connector type [2]
[2]. https://patchwork.kernel.org/patch/10914379/

The  purpose of "usb role switch" is to  assign the roles. So looks like, we don't need this function.
Please correct me, if I am wrong.

Regards,
Biju 
> > +{
> > +	struct device_node *np = dev->of_node;
> > +	struct device_node *parent;
> > +	struct device_node *child;
> > +	bool ret = false;
> > +	const char *role_type = NULL;
> > +
> > +	child = of_graph_get_endpoint_by_regs(np, -1, -1);
> > +	if (!child)
> > +		return ret;
> > +
> > +	parent = of_graph_get_remote_port_parent(child);
> > +	of_node_put(child);
> > +	child = of_get_child_by_name(parent, "connector");
> > +	of_node_put(parent);
> > +	if (!child)
> > +		return ret;
> > +
> > +	if (of_device_is_compatible(child, "usb-c-connector")) {
> > +		of_property_read_string(child, "data-role", &role_type);
> > +		if (role_type && (!strncmp(role_type, "dual",
> strlen("dual"))))
> > +			ret = true;
> > +	}
> > +
> > +	of_node_put(child);
> > +	return ret;
> > +}
> > +
> >  static int renesas_usb3_probe(struct platform_device *pdev)  {
> >  	struct renesas_usb3 *usb3;
> > @@ -2741,6 +2842,12 @@ static int renesas_usb3_probe(struct
> platform_device *pdev)
> >  	if (ret < 0)
> >  		goto err_dev_create;
> >
> > +	if (device_property_read_bool(&pdev->dev, "usb-role-switch") &&
> > +	    is_usb_dual_role_switch(&pdev->dev)) {
> > +		usb3->dual_role_sw = 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);
> 

RE: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role switch support

[Yoshihiro Shimoda]

Hi Biju-san,

Thank you for the patch!

> From: Biju Das, Sent: Wednesday, May 15, 2019 9:09 PM
> Subject: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role switch support

Now I'm confusing about the "Add dual role switch support" mean... Especially,
this driver has already supports dual role switch support by own sysfs or debugfs.

> The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220 usb
> type-c drp port controller. This patch adds dual role switch support for
> the type-c connector using the usb role switch class framework.

IIUC, after this patch is applied, the hs3ss3220 type-c driver can switch
the role by using the usb role switch class framework.

> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> ---
>  V5-->V6
>    * Added graph api's to find the role supported by the connector.
>  V4-->V5
>    * Incorporated Shimoda-san's review comment
>     (https://patchwork.kernel.org/patch/10902537/)
>  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 | 121 ++++++++++++++++++++++++++++++++--
>  1 file changed, 114 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
> index 7dc2485..1d41998 100644
> --- a/drivers/usb/gadget/udc/renesas_usb3.c
> +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> @@ -24,6 +24,7 @@
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/of.h>
> +#include <linux/of_graph.h>
>  #include <linux/usb/role.h>
> 
>  /* register definitions */
> @@ -351,6 +352,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 +362,7 @@ struct renesas_usb3 {
>  	bool extcon_usb;		/* check vbus and set EXTCON_USB */
>  	bool forced_b_device;
>  	bool start_to_connect;
> +	bool dual_role_sw;

I'm also confusing this name.

>  };
> 
>  #define gadget_to_renesas_usb3(_gadget)	\
<snip>
> @@ -699,8 +703,10 @@ 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->dual_role_sw || 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);
> @@ -716,7 +722,8 @@ static void usb3_check_id(struct renesas_usb3 *usb3)
>  {
>  	usb3->extcon_host = usb3_is_a_device(usb3);
> 
> -	if (usb3->extcon_host && !usb3->forced_b_device)
> +	if ((!usb3->dual_role_sw && usb3->extcon_host && !usb3->forced_b_device)
> +	    || usb3->connection_state == USB_ROLE_HOST)
>  		usb3_mode_config(usb3, true, true);
>  	else
>  		usb3_mode_config(usb3, false, false);
> @@ -2343,14 +2350,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 +2419,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->dual_role_sw)
> +		handle_ext_role_switch_states(dev, role);
> +	else
> +		handle_role_switch_states(dev, role);
> +
>  	pm_runtime_put(dev);
> 
>  	return 0;
> @@ -2650,12 +2722,41 @@ 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,
>  };
> 
> +static bool is_usb_dual_role_switch(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct device_node *parent;
> +	struct device_node *child;
> +	bool ret = false;
> +	const char *role_type = NULL;
> +
> +	child = of_graph_get_endpoint_by_regs(np, -1, -1);
> +	if (!child)
> +		return ret;
> +
> +	parent = of_graph_get_remote_port_parent(child);
> +	of_node_put(child);
> +	child = of_get_child_by_name(parent, "connector");
> +	of_node_put(parent);
> +	if (!child)
> +		return ret;
> +
> +	if (of_device_is_compatible(child, "usb-c-connector")) {
> +		of_property_read_string(child, "data-role", &role_type);
> +		if (role_type && (!strncmp(role_type, "dual", strlen("dual"))))
> +			ret = true;
> +	}
> +
> +	of_node_put(child);
> +	return ret;
> +}
> +
>  static int renesas_usb3_probe(struct platform_device *pdev)
>  {
>  	struct renesas_usb3 *usb3;
> @@ -2741,6 +2842,12 @@ static int renesas_usb3_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_dev_create;
> 
> +	if (device_property_read_bool(&pdev->dev, "usb-role-switch") &&
> +	    is_usb_dual_role_switch(&pdev->dev)) {

I think either one of the conditions is enough. (Only "usb-role-switch"
checking is enough, IIUC).

JFYI, according to the binding document [1], this "usb-role-switch" means:
---
+ - usb-role-switch: boolean, indicates that the device is capable of assigning
+			the USB data role (USB host or USB device) for a given
+			USB connector, such as Type-C, Type-B(micro).
+			see connector/usb-connector.txt.
---

So, R-Car Gen3 / Salvator-XS cannot have this property because the board
has Type-A connector.

[1] https://patchwork.kernel.org/patch/10934835/

> +		usb3->dual_role_sw = true;

So, "role_sw_by_connector" matches with my image.
What do you think?

Best regards,
Yoshihiro Shimoda

> +		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

RE: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role switch support

[Biju Das]

Hi Shimoda-San,

Thanks for the feedback.

> > From: Biju Das, Sent: Wednesday, May 15, 2019 9:09 PM
> > Subject: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role
> > switch support
> 
> Now I'm confusing about the "Add dual role switch support" mean...
> Especially, this driver has already supports dual role switch support by own
> sysfs or debugfs.

Sorry for the confusion. 
What about "Enhance role switch support" ?

> > The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220
> > usb type-c drp port controller. This patch adds dual role switch
> > support for the type-c connector using the usb role switch class framework.
> 
> IIUC, after this patch is applied, the hs3ss3220 type-c driver can switch the
> role by using the usb role switch class framework.

Yes, That is correct. HD3SS3220 driver detects host/device  connection events (attach/detach) and 
It calls "usb_role_switch_set_role"  to assign/switch the role.

> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > ---
> >  V5-->V6
> >    * Added graph api's to find the role supported by the connector.
> >  V4-->V5
> >    * Incorporated Shimoda-san's review comment
> >     (https://patchwork.kernel.org/patch/10902537/)
> >  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 | 121
> > ++++++++++++++++++++++++++++++++--
> >  1 file changed, 114 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c
> > b/drivers/usb/gadget/udc/renesas_usb3.c
> > index 7dc2485..1d41998 100644
> > --- a/drivers/usb/gadget/udc/renesas_usb3.c
> > +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/usb/ch9.h>
> >  #include <linux/usb/gadget.h>
> >  #include <linux/usb/of.h>
> > +#include <linux/of_graph.h>
> >  #include <linux/usb/role.h>
> >
> >  /* register definitions */
> > @@ -351,6 +352,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 +362,7 @@ struct renesas_usb3 {
> >  	bool extcon_usb;		/* check vbus and set EXTCON_USB
> */
> >  	bool forced_b_device;
> >  	bool start_to_connect;
> > +	bool dual_role_sw;
> 
> I'm also confusing this name.

OK. will change it to "role_sw_by_connector"

> >  };
> >
> >  #define gadget_to_renesas_usb3(_gadget)	\
> <snip>
> > @@ -699,8 +703,10 @@ 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->dual_role_sw || 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);
> > @@ -716,7 +722,8 @@ static void usb3_check_id(struct renesas_usb3
> > *usb3)  {
> >  	usb3->extcon_host = usb3_is_a_device(usb3);
> >
> > -	if (usb3->extcon_host && !usb3->forced_b_device)
> > +	if ((!usb3->dual_role_sw && usb3->extcon_host && !usb3-
> >forced_b_device)
> > +	    || usb3->connection_state == USB_ROLE_HOST)
> >  		usb3_mode_config(usb3, true, true);
> >  	else
> >  		usb3_mode_config(usb3, false, false); @@ -2343,14 +2350,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 +2419,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->dual_role_sw)
> > +		handle_ext_role_switch_states(dev, role);
> > +	else
> > +		handle_role_switch_states(dev, role);
> > +
> >  	pm_runtime_put(dev);
> >
> >  	return 0;
> > @@ -2650,12 +2722,41 @@ 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,
> >  };
> >
> > +static bool is_usb_dual_role_switch(struct device *dev) {
> > +	struct device_node *np = dev->of_node;
> > +	struct device_node *parent;
> > +	struct device_node *child;
> > +	bool ret = false;
> > +	const char *role_type = NULL;
> > +
> > +	child = of_graph_get_endpoint_by_regs(np, -1, -1);
> > +	if (!child)
> > +		return ret;
> > +
> > +	parent = of_graph_get_remote_port_parent(child);
> > +	of_node_put(child);
> > +	child = of_get_child_by_name(parent, "connector");
> > +	of_node_put(parent);
> > +	if (!child)
> > +		return ret;
> > +
> > +	if (of_device_is_compatible(child, "usb-c-connector")) {
> > +		of_property_read_string(child, "data-role", &role_type);
> > +		if (role_type && (!strncmp(role_type, "dual",
> strlen("dual"))))
> > +			ret = true;
> > +	}
> > +
> > +	of_node_put(child);
> > +	return ret;
> > +}
> > +
> >  static int renesas_usb3_probe(struct platform_device *pdev)  {
> >  	struct renesas_usb3 *usb3;
> > @@ -2741,6 +2842,12 @@ static int renesas_usb3_probe(struct
> platform_device *pdev)
> >  	if (ret < 0)
> >  		goto err_dev_create;
> >
> > +	if (device_property_read_bool(&pdev->dev, "usb-role-switch") &&
> > +	    is_usb_dual_role_switch(&pdev->dev)) {
> 
> I think either one of the conditions is enough. (Only "usb-role-switch"
> checking is enough, IIUC).

OK, Will remove the other check.

> JFYI, according to the binding document [1], this "usb-role-switch" means:
> ---
> + - usb-role-switch: boolean, indicates that the device is capable of assigning
> +			the USB data role (USB host or USB device) for a
> given
> +			USB connector, such as Type-C, Type-B(micro).
> +			see connector/usb-connector.txt.
> ---
> 
> So, R-Car Gen3 / Salvator-XS cannot have this property because the board
> has Type-A connector.
> 
> [1] https://patchwork.kernel.org/patch/10934835/

I have updated the  binding patch to cover this[1]
[1]. https://patchwork.kernel.org/patch/10944631/
Are you ok with this binding patch?

> > +		usb3->dual_role_sw = true;
> 
> So, "role_sw_by_connector" matches with my image.
> What do you think?

OK, will use " role_sw_by_connector"

Regards,
Biju
 
> > +		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

RE: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role switch support

[Yoshihiro Shimoda]

Hi Biju-san,

> From: Biju Das, Sent: Tuesday, May 21, 2019 4:10 PM
> 
> Hi Shimoda-San,
> 
> Thanks for the feedback.
> 
> > > From: Biju Das, Sent: Wednesday, May 15, 2019 9:09 PM
> > > Subject: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role
> > > switch support
> >
> > Now I'm confusing about the "Add dual role switch support" mean...
> > Especially, this driver has already supports dual role switch support by own
> > sysfs or debugfs.
> 
> Sorry for the confusion.
> What about "Enhance role switch support" ?

Thank you for the suggestion. It's good to me.

> > > The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220
> > > usb type-c drp port controller. This patch adds dual role switch
> > > support for the type-c connector using the usb role switch class framework.
> >
> > IIUC, after this patch is applied, the hs3ss3220 type-c driver can switch the
> > role by using the usb role switch class framework.
> 
> Yes, That is correct. HD3SS3220 driver detects host/device  connection events (attach/detach) and
> It calls "usb_role_switch_set_role"  to assign/switch the role.

I got it.

<snip>
> > JFYI, according to the binding document [1], this "usb-role-switch" means:
> > ---
> > + - usb-role-switch: boolean, indicates that the device is capable of assigning
> > +			the USB data role (USB host or USB device) for a
> > given
> > +			USB connector, such as Type-C, Type-B(micro).
> > +			see connector/usb-connector.txt.
> > ---
> >
> > So, R-Car Gen3 / Salvator-XS cannot have this property because the board
> > has Type-A connector.
> >
> > [1] https://patchwork.kernel.org/patch/10934835/
> 
> I have updated the  binding patch to cover this[1]
> [1]. https://patchwork.kernel.org/patch/10944631/
> Are you ok with this binding patch?

I'm sorry, I overlooked the patch. I'll check it later.

Best regards,
Yoshihiro Shimoda

RE: [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding document

[Yoshihiro Shimoda]

Hi Biju-san,

Thank you for the patch!

> From: Biju Das, Sent: Wednesday, May 15, 2019 9:09 PM
> 
> 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>
> ---
> V5-->V6
>   * No change.
> V4-->V5
>   * No Change.
> 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

s/connector :/connector:/

> +   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>;
> +		};
> +	};

According to the connector/usb-connector.txt, should the connector node
have ports, port@1 and an endpoint nodes like below?

	usb_con: connector {
		compatible = "usb-c-connector";
		label = "USB-C";
		data-role = "dual";

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@1 {
				reg = <1>;
				hd3ss3220_ep: endpoint {
					remote-endpoint = <&usb3peri_role_switch>;
				};
			};
		};
	};

Best regards,
Yoshihiro Shimoda

Re: [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding document

[Kuninori Morimoto]

Hi

> > +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
> 
> s/connector :/connector:/

Maybe it is for alignment ?

> According to the connector/usb-connector.txt, should the connector node
> have ports, port@1 and an endpoint nodes like below?

"ports" is needed if it has multiple "port",
otherwise, single port is allowed from OF-graph point of view.

Thank you for your help !!
Best regards
---
Kuninori Morimoto

RE: [PATCH v6 2/7] dt-bindings: usb: renesas_usb3: Document usb role switch support

[Yoshihiro Shimoda]

Hi Biju-san,

Thank you for the patch!

> From: Biju Das, Sent: Wednesday, May 15, 2019 9:09 PM
> 
> Update the DT bindings documentation to support usb role switch
> for USB Type-C connector using USB role switch class framework.
> 
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> ---
>  V5-->V6
>   * Updated description
>   * Added usb-role-switch-property
>  V4-->V5
>   * No Change
>  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       | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> index 35039e7..ea6c63c 100644
> --- a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> +++ b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> @@ -22,6 +22,11 @@ Required properties:
>  Optional properties:
>    - phys: phandle + phy specifier pair
>    - phy-names: must be "usb"
> +  - usb-role-switch: support role switch. see usb/generic.txt
> +
> +Sub-nodes:
> +The port would be added as a subnode if the "usb-role-switch" property is used.
> +	see graph.txt

I think we should describe which type of a subnode is needed.
I made an example below. This is based on the usb-connector.txt.
---
Sub-nodes:
- any connector to the data bus of this controller should be modeled using the OF graph
  bindings specified in bindings/graph.txt.
---

>  Example of R-Car H3 ES1.x:
>  	usb3_peri0: usb@ee020000 {
> @@ -39,3 +44,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>;
> +		usb-role-switch;
> +
> +		port {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			usb3peri_role_switch: endpoint@0 {
> +				reg = <0>;

I'm not sure, but I don't think this endpoint@0 and reg = <0> are needed.
In other words, can we use have following node?
			usb3peri_role_switch: endpoint {
				remote-endpoint = <&hd3ss3220_ep>;
			};

Best regards,
Yoshihiro Shimoda

RE: [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding document

[Biju Das]

Hi Sergei,

Thanks for the feedback.

> Subject: Re: [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding
> document
> 
> Hello!
> 
> On 05/15/2019 03:09 PM, Biju Das wrote:
> 
> > 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>
> > ---
> > V5-->V6
> >   * No change.
> > V4-->V5
> >   * No Change.
> > 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.
> 
>    This depends on an interrupt controller used. I'd just said "an interrupt
> specifier", w/o further details.

Fine , If it is ok for everyone. 

Regards,
Biju
> > +
> > +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
> > +
> [...]
> 
> MBR, Sergei

RE: [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding document

[Biju Das]

Hi Morimoto-San and Shimoda-San,

Thanks for the feedback.

> Subject: Re: [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding
> document
> 
> 
> Hi
> 
> > > +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
> >
> > s/connector :/connector:/
> 
> Maybe it is for alignment ?

Yes, I need to fix the extra space.

> > According to the connector/usb-connector.txt, should the connector
> > node have ports, port@1 and an endpoint nodes like below?
> 
> "ports" is needed if it has multiple "port", otherwise, single port is allowed
> from OF-graph point of view.

OK. I will use single port on  the next patch series.

Regards,
Biju

RE: [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding document

[Yoshihiro Shimoda]

Hi Biju-san, Morimoto-san,

> From: Biju Das, Sent: Wednesday, May 22, 2019 8:00 PM
<snip>
> > > According to the connector/usb-connector.txt, should the connector
> > > node have ports, port@1 and an endpoint nodes like below?
> >
> > "ports" is needed if it has multiple "port", otherwise, single port is allowed
> > from OF-graph point of view.
> 
> OK. I will use single port on  the next patch series.

According to the connector/usb-connector.txt [1], even if this device uses a single port,
we should describe ports node and port@1 (for SuperSpeed) subnode like usb/typec-tcpci.txt.

[1]
Required nodes:
- any data bus to the connector should be modeled using the OF graph bindings
  specified in bindings/graph.txt, unless the bus is between parent node and
  the connector. Since single connector can have multiple data buses every bus
  has assigned OF graph port number as follows:
    0: High Speed (HS), present in all connectors,
    1: Super Speed (SS), present in SS capable connectors,
    2: Sideband use (SBU), present in USB-C.

Best regards,
Yoshihiro Shimoda

RE: [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding document

[Biju Das]

Hi Shimoda-San,

Thanks for the feedback.

> Subject: RE: [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding
> document
> 
> Hi Biju-san, Morimoto-san,
> 
> > From: Biju Das, Sent: Wednesday, May 22, 2019 8:00 PM
> <snip>
> > > > According to the connector/usb-connector.txt, should the connector
> > > > node have ports, port@1 and an endpoint nodes like below?
> > >
> > > "ports" is needed if it has multiple "port", otherwise, single port
> > > is allowed from OF-graph point of view.
> >
> > OK. I will use single port on  the next patch series.
> 
> According to the connector/usb-connector.txt [1], even if this device uses a
> single port, we should describe ports node and port@1 (for SuperSpeed)
> subnode like usb/typec-tcpci.txt.

OK.  I will update the example like below.

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";

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@1 {
				reg = <1>;
				hd3ss3220_ep: endpoint {
					remote-endpoint = <&usb3_role_switch>;
				};
			};
		};
	};
};

Regards,
Biju

> [1]
> Required nodes:
> - any data bus to the connector should be modeled using the OF graph
> bindings
>   specified in bindings/graph.txt, unless the bus is between parent node and
>   the connector. Since single connector can have multiple data buses every
> bus
>   has assigned OF graph port number as follows:
>     0: High Speed (HS), present in all connectors,
>     1: Super Speed (SS), present in SS capable connectors,
>     2: Sideband use (SBU), present in USB-C.

Re: [PATCH v6 7/7] arm64: dts: renesas: r8a774c0-cat874: Enable usb role switch support

[Rob Herring]

On Wed, May 15, 2019 at 01:09:12PM +0100, Biju Das wrote:
> 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>
> ---
> V5-->V6
>   * No change
> V4-->V5
>   * No change
> 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..124ed58 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 {

port should be a child of 'connector' node. It should also be port #1 if 
this is a SuperSpeed controller. Port #0 is HS.

As there are multiple ports possible, there should be a 'ports' node 
too.

> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			hd3ss3220_ep: endpoint@0 {
> +				reg = <0>;

Don't need reg when there is only 1. Build your dtb with W=1 as that 
will tell you this.

> +				remote-endpoint = <&usb3peri_role_switch>;
> +			};
> +		};
> +	};
> +};
> +
>  &i2c1 {
>  	pinctrl-0 = <&i2c1_pins>;
>  	pinctrl-names = "default";
> @@ -175,6 +203,17 @@
>  &usb3_peri0 {
>  	companion = <&xhci0>;
>  	status = "okay";
> +	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
> 

RE: [PATCH v6 2/7] dt-bindings: usb: renesas_usb3: Document usb role switch support

[Biju Das]

HI Shimoda-San,

Thanks for the feedback.

> Subject: RE: [PATCH v6 2/7] dt-bindings: usb: renesas_usb3: Document usb
> role switch support
> 
> Hi Biju-san,
> 
> Thank you for the patch!
> 
> > From: Biju Das, Sent: Wednesday, May 15, 2019 9:09 PM
> >
> > Update the DT bindings documentation to support usb role switch for
> > USB Type-C connector using USB role switch class framework.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > ---
> >  V5-->V6
> >   * Updated description
> >   * Added usb-role-switch-property
> >  V4-->V5
> >   * No Change
> >  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       | 26
> ++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > index 35039e7..ea6c63c 100644
> > --- a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > +++ b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > @@ -22,6 +22,11 @@ Required properties:
> >  Optional properties:
> >    - phys: phandle + phy specifier pair
> >    - phy-names: must be "usb"
> > +  - usb-role-switch: support role switch. see usb/generic.txt
> > +
> > +Sub-nodes:
> > +The port would be added as a subnode if the "usb-role-switch" property is
> used.
> > +	see graph.txt
> 
> I think we should describe which type of a subnode is needed.
> I made an example below. This is based on the usb-connector.txt.
> ---
> Sub-nodes:
> - any connector to the data bus of this controller should be modeled using
> the OF graph
> bindings specified in bindings/graph.txt

OK to me. I will add the following  at the end.  ", if the "usb-role-switch" property is
used."
> ---
> 
> >  Example of R-Car H3 ES1.x:
> >  	usb3_peri0: usb@ee020000 {
> > @@ -39,3 +44,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>;
> > +		usb-role-switch;
> > +
> > +		port {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			usb3peri_role_switch: endpoint@0 {
> > +				reg = <0>;
> 
> I'm not sure, but I don't think this endpoint@0 and reg = <0> are needed.
> In other words, can we use have following node?
> 			usb3peri_role_switch: endpoint {
> 				remote-endpoint = <&hd3ss3220_ep>;
> 			};

OK . Will update  the example with

        port {                                                                   
                usb3_role_switch: endpoint {                                     
                        remote-endpoint = <&hd3ss3220_ep>;                       
                 };                                                               
         };

Regards,
Biju