[PATCH/RFC v3 0/4] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs

[PATCH/RFC v3 0/4] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs

[Yoshihiro Shimoda]

This patch set is based on Felipe's usb.git / testing/next branch
(commit id = 5d1332a8eabd8bd5b8c322d45542968ee6f113be).

I still marked this patch set as "RFC". I would like to know whether
this way is good or not.

Changes from RFC v2:
 - Add registering usb role switch into drivers/usb/gadget/udc/renesas_usb3
   because hardware resource (a register) is shared and remove individual
   usb role switch driver/dt-bindings for R-Car.
 - Remove "usb_role_switch_get_by_graph" API because the renesas_usb3 driver
   doesn't need such API now.

Changes from RFC:
 - Remove "device-connection-id" and "usb role switch driver" dt-bingings.
 - Remove drivers/of code.
 - Add a new API for find the connection by using graph on devcon.c and roles.c.
 - Use each new API on the rcar usb role switch and renesas_usb3 drivers.
 - Update the dtsi file for r8a7795.


Yoshihiro Shimoda (4):
  base: devcon: add a new API to find the graph
  usb: gadget: udc: renesas_usb3: Add register of usb role switch
  usb: gadget: udc: renesas_usb3: use usb role switch API
  arm64: dts: renesas: r8a7795: add OF graph for usb role switch

 .../devicetree/bindings/usb/renesas_usb3.txt       | 15 +++++
 Documentation/driver-api/device_connection.rst     |  4 +-
 arch/arm64/boot/dts/renesas/r8a7795.dtsi           | 12 ++++
 drivers/base/devcon.c                              | 43 +++++++++++++
 drivers/usb/gadget/udc/Kconfig                     |  1 +
 drivers/usb/gadget/udc/renesas_usb3.c              | 74 +++++++++++++++++++++-
 include/linux/device.h                             |  2 +
 7 files changed, 147 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH/RFC v3 1/4] base: devcon: add a new API to find the graph

[Yoshihiro Shimoda]

This patch adds a new API "device_connection_find_by_graph()" to
find device connection by using graph.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Documentation/driver-api/device_connection.rst |  4 +--
 drivers/base/devcon.c                          | 43 ++++++++++++++++++++++++++
 include/linux/device.h                         |  2 ++
 3 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/Documentation/driver-api/device_connection.rst b/Documentation/driver-api/device_connection.rst
index affbc556..2e2d26f 100644
--- a/Documentation/driver-api/device_connection.rst
+++ b/Documentation/driver-api/device_connection.rst
@@ -19,7 +19,7 @@ Device connections alone do not create a dependency between the two devices.
 They are only descriptions which are not tied to either of the devices directly.
 A dependency between the two devices exists only if one of the two endpoint
 devices requests a reference to the other. The descriptions themselves can be
-defined in firmware (not yet supported) or they can be built-in.
+defined in firmware or they can be built-in.
 
 Usage
 -----
@@ -40,4 +40,4 @@ API
 ---
 
 .. kernel-doc:: drivers/base/devcon.c
-   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove
+   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove device_connection_find_by_graph
diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index d427e80..5a0da33 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/property.h>
 
 static DEFINE_MUTEX(devcon_lock);
 static LIST_HEAD(devcon_list);
@@ -134,3 +135,45 @@ void device_connection_remove(struct device_connection *con)
 	mutex_unlock(&devcon_lock);
 }
 EXPORT_SYMBOL_GPL(device_connection_remove);
+
+static int generic_graph_match(struct device *dev, void *fwnode)
+{
+	return dev->fwnode == fwnode;
+}
+
+/**
+ * device_connection_find_by_graph - Find two devices connected together
+ * @dev: Device to find connected device
+ * @port: identifier of the @dev port node
+ * @endpoint: identifier of the @dev endpoint node
+ *
+ * Find a connection with @port and @endpoint by using graph between @dev and
+ * another device. On success returns handle to the device that is connected
+ * to @dev, with the reference count for the found device incremented. Returns
+ * NULL if no matching connection was found, or ERR_PTR(-EPROBE_DEFER) when
+ * a connection was found but the other device has not been enumerated yet.
+ */
+struct device *device_connection_find_by_graph(struct device *dev, u32 port,
+					       u32 endpoint)
+{
+	struct bus_type *bus;
+	struct fwnode_handle *remote;
+	struct device *conn;
+
+	remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
+	if (!remote)
+		return NULL;
+
+	for (bus = generic_match_buses[0]; bus; bus++) {
+		conn = bus_find_device(bus, NULL, remote, generic_graph_match);
+		if (conn)
+			return conn;
+	}
+
+	/*
+	 * We only get called if a connection was found, tell the caller to
+	 * wait for the other device to show up.
+	 */
+	return ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL_GPL(device_connection_find_by_graph);
diff --git a/include/linux/device.h b/include/linux/device.h
index 0059b99..58f8544 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -751,6 +751,8 @@ void *device_connection_find_match(struct device *dev, const char *con_id,
 
 void device_connection_add(struct device_connection *con);
 void device_connection_remove(struct device_connection *con);
+struct device *device_connection_find_by_graph(struct device *dev, u32 port,
+					       u32 endpoint);
 
 /**
  * enum device_link_state - Device link states.
-- 
1.9.1

[PATCH/RFC,v3,1/4] base: devcon: add a new API to find the graph

[Yoshihiro Shimoda]

This patch adds a new API "device_connection_find_by_graph()" to
find device connection by using graph.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Documentation/driver-api/device_connection.rst |  4 +--
 drivers/base/devcon.c                          | 43 ++++++++++++++++++++++++++
 include/linux/device.h                         |  2 ++
 3 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/Documentation/driver-api/device_connection.rst b/Documentation/driver-api/device_connection.rst
index affbc556..2e2d26f 100644
--- a/Documentation/driver-api/device_connection.rst
+++ b/Documentation/driver-api/device_connection.rst
@@ -19,7 +19,7 @@ Device connections alone do not create a dependency between the two devices.
 They are only descriptions which are not tied to either of the devices directly.
 A dependency between the two devices exists only if one of the two endpoint
 devices requests a reference to the other. The descriptions themselves can be
-defined in firmware (not yet supported) or they can be built-in.
+defined in firmware or they can be built-in.
 
 Usage
 -----
@@ -40,4 +40,4 @@ API
 ---
 
 .. kernel-doc:: drivers/base/devcon.c
-   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove
+   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove device_connection_find_by_graph
diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index d427e80..5a0da33 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/property.h>
 
 static DEFINE_MUTEX(devcon_lock);
 static LIST_HEAD(devcon_list);
@@ -134,3 +135,45 @@ void device_connection_remove(struct device_connection *con)
 	mutex_unlock(&devcon_lock);
 }
 EXPORT_SYMBOL_GPL(device_connection_remove);
+
+static int generic_graph_match(struct device *dev, void *fwnode)
+{
+	return dev->fwnode == fwnode;
+}
+
+/**
+ * device_connection_find_by_graph - Find two devices connected together
+ * @dev: Device to find connected device
+ * @port: identifier of the @dev port node
+ * @endpoint: identifier of the @dev endpoint node
+ *
+ * Find a connection with @port and @endpoint by using graph between @dev and
+ * another device. On success returns handle to the device that is connected
+ * to @dev, with the reference count for the found device incremented. Returns
+ * NULL if no matching connection was found, or ERR_PTR(-EPROBE_DEFER) when
+ * a connection was found but the other device has not been enumerated yet.
+ */
+struct device *device_connection_find_by_graph(struct device *dev, u32 port,
+					       u32 endpoint)
+{
+	struct bus_type *bus;
+	struct fwnode_handle *remote;
+	struct device *conn;
+
+	remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
+	if (!remote)
+		return NULL;
+
+	for (bus = generic_match_buses[0]; bus; bus++) {
+		conn = bus_find_device(bus, NULL, remote, generic_graph_match);
+		if (conn)
+			return conn;
+	}
+
+	/*
+	 * We only get called if a connection was found, tell the caller to
+	 * wait for the other device to show up.
+	 */
+	return ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL_GPL(device_connection_find_by_graph);
diff --git a/include/linux/device.h b/include/linux/device.h
index 0059b99..58f8544 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -751,6 +751,8 @@ void *device_connection_find_match(struct device *dev, const char *con_id,
 
 void device_connection_add(struct device_connection *con);
 void device_connection_remove(struct device_connection *con);
+struct device *device_connection_find_by_graph(struct device *dev, u32 port,
+					       u32 endpoint);
 
 /**
  * enum device_link_state - Device link states.

[PATCH/RFC v3 2/4] usb: gadget: udc: renesas_usb3: Add register of usb role switch

[Yoshihiro Shimoda]

This patch adds role switch support for R-Car SoCs into the USB
3.0 peripheral driver. Some R-Car SoCs (e.g. R-Car H3) have USB
3.0 dual-role device controller which has the USB 3.0 xHCI host
and Renesas USB 3.0 peripheral.

Unfortunately, the mode change register contains the USB 3.0 peripheral
controller side only. So, the USB 3.0 peripheral driver (renesas_usb3)
manages this register now. However, in peripheral mode, the host
should stop. Also the host hardware needs to reinitialize its own
registers when the mode changes from peripheral to host mode.
Otherwise, the host cannot work correctly (e.g. detect a device as
high-speed).

To achieve this by a driver, this role switch driver manages
the mode change register and attach/release the xhci-plat driver.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 .../devicetree/bindings/usb/renesas_usb3.txt       | 15 +++++
 drivers/usb/gadget/udc/Kconfig                     |  1 +
 drivers/usb/gadget/udc/renesas_usb3.c              | 68 +++++++++++++++++++++-
 3 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
index 2c071bb5..f6105aa 100644
--- a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
+++ b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
@@ -19,6 +19,9 @@ Required properties:
 Optional properties:
   - phys: phandle + phy specifier pair
   - phy-names: must be "usb"
+  - The connection to a usb3.0 host node needs by using OF graph bindings for
+    usb role switch.
+   - port@0 = USB3.0 host port.
 
 Example of R-Car H3 ES1.x:
 	usb3_peri0: usb@ee020000 {
@@ -27,6 +30,12 @@ Example of R-Car H3 ES1.x:
 		reg = <0 0xee020000 0 0x400>;
 		interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cpg CPG_MOD 328>;
+
+		port {
+			usb3_peri0_ep: endpoint {
+				remote-endpoint = <&usb3_host0_ep>;
+			};
+		};
 	};
 
 	usb3_peri1: usb@ee060000 {
@@ -35,4 +44,10 @@ Example of R-Car H3 ES1.x:
 		reg = <0 0xee060000 0 0x400>;
 		interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cpg CPG_MOD 327>;
+
+		port {
+			usb3_peri1_ep: endpoint {
+				remote-endpoint = <&usb3_host1_ep>;
+			};
+		};
 	};
diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index 0875d38..7e4a5dd 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -193,6 +193,7 @@ config USB_RENESAS_USB3
 	tristate 'Renesas USB3.0 Peripheral controller'
 	depends on ARCH_RENESAS || COMPILE_TEST
 	depends on EXTCON && HAS_DMA
+	select USB_ROLE_SWITCH
 	help
 	   Renesas USB3.0 Peripheral controller is a USB peripheral controller
 	   that supports super, high, and full speed USB 3.0 data transfers.
diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
index 409cde4..c878449 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -23,6 +23,7 @@
 #include <linux/uaccess.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
+#include <linux/usb/role.h>
 
 /* register definitions */
 #define USB3_AXI_INT_STA	0x008
@@ -334,6 +335,9 @@ struct renesas_usb3 {
 	struct work_struct extcon_work;
 	struct phy *phy;
 
+	struct usb_role_switch *role_sw;
+	struct device *host_dev;
+
 	struct renesas_usb3_ep *usb3_ep;
 	int num_usb3_eps;
 
@@ -643,7 +647,7 @@ static void usb3_check_vbus(struct renesas_usb3 *usb3)
 	}
 }
 
-static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
+static void _usb3_set_mode(struct renesas_usb3 *usb3, bool host)
 {
 	if (host)
 		usb3_clear_bit(usb3, DRD_CON_PERI_CON, USB3_DRD_CON);
@@ -651,6 +655,11 @@ static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
 		usb3_set_bit(usb3, DRD_CON_PERI_CON, USB3_DRD_CON);
 }
 
+static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
+{
+	_usb3_set_mode(usb3, host);
+}
+
 static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
 {
 	if (enable)
@@ -2294,6 +2303,41 @@ static int renesas_usb3_set_selfpowered(struct usb_gadget *gadget, int is_self)
 	.set_selfpowered	= renesas_usb3_set_selfpowered,
 };
 
+static enum usb_role renesas_usb3_role_switch_get(struct device *dev)
+{
+	struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
+	enum usb_role cur_role;
+
+	pm_runtime_get_sync(dev);
+	cur_role = usb3_is_host(usb3) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
+	pm_runtime_put(dev);
+
+	return cur_role;
+}
+
+static int renesas_usb3_role_switch_set(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);
+	} else if (cur_role == USB_ROLE_DEVICE && role == USB_ROLE_HOST) {
+		/* Must set the mode before device_attach of the host */
+		_usb3_set_mode(usb3, true);
+		if (device_attach(host) < 0)
+			dev_err(dev, "device_attach(usb3_port) failed\n");
+	}
+	pm_runtime_put(dev);
+
+	return 0;
+}
+
 static ssize_t role_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
@@ -2404,6 +2448,8 @@ static int renesas_usb3_remove(struct platform_device *pdev)
 
 	device_remove_file(&pdev->dev, &dev_attr_role);
 
+	usb_role_switch_unregister(usb3->role_sw);
+
 	usb_del_gadget_udc(&usb3->gadget);
 	renesas_usb3_dma_free_prd(usb3, &pdev->dev);
 
@@ -2562,6 +2608,12 @@ static void renesas_usb3_init_ram(struct renesas_usb3 *usb3, struct device *dev,
 	EXTCON_NONE,
 };
 
+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 int renesas_usb3_probe(struct platform_device *pdev)
 {
 	struct renesas_usb3 *usb3;
@@ -2644,6 +2696,20 @@ static int renesas_usb3_probe(struct platform_device *pdev)
 	if (IS_ERR(usb3->phy))
 		usb3->phy = NULL;
 
+	usb3->role_sw = usb_role_switch_register(&pdev->dev,
+					&renesas_usb3_role_switch_desc);
+	if (!IS_ERR(usb3->role_sw)) {
+		usb3->host_dev = device_connection_find_by_graph(&pdev->dev,
+								 0, 0);
+		if (IS_ERR_OR_NULL(usb3->host_dev)) {
+			/* If not found, this driver will not use a role sw */
+			usb_role_switch_unregister(usb3->role_sw);
+			usb3->role_sw = NULL;
+		}
+	} else {
+		usb3->role_sw = NULL;
+	}
+
 	usb3->workaround_for_vbus = priv->workaround_for_vbus;
 
 	renesas_usb3_debugfs_init(usb3, &pdev->dev);
-- 
1.9.1

[PATCH/RFC,v3,2/4] usb: gadget: udc: renesas_usb3: Add register of usb role switch

[Yoshihiro Shimoda]

This patch adds role switch support for R-Car SoCs into the USB
3.0 peripheral driver. Some R-Car SoCs (e.g. R-Car H3) have USB
3.0 dual-role device controller which has the USB 3.0 xHCI host
and Renesas USB 3.0 peripheral.

Unfortunately, the mode change register contains the USB 3.0 peripheral
controller side only. So, the USB 3.0 peripheral driver (renesas_usb3)
manages this register now. However, in peripheral mode, the host
should stop. Also the host hardware needs to reinitialize its own
registers when the mode changes from peripheral to host mode.
Otherwise, the host cannot work correctly (e.g. detect a device as
high-speed).

To achieve this by a driver, this role switch driver manages
the mode change register and attach/release the xhci-plat driver.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 .../devicetree/bindings/usb/renesas_usb3.txt       | 15 +++++
 drivers/usb/gadget/udc/Kconfig                     |  1 +
 drivers/usb/gadget/udc/renesas_usb3.c              | 68 +++++++++++++++++++++-
 3 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
index 2c071bb5..f6105aa 100644
--- a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
+++ b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
@@ -19,6 +19,9 @@ Required properties:
 Optional properties:
   - phys: phandle + phy specifier pair
   - phy-names: must be "usb"
+  - The connection to a usb3.0 host node needs by using OF graph bindings for
+    usb role switch.
+   - port@0 = USB3.0 host port.
 
 Example of R-Car H3 ES1.x:
 	usb3_peri0: usb@ee020000 {
@@ -27,6 +30,12 @@ Example of R-Car H3 ES1.x:
 		reg = <0 0xee020000 0 0x400>;
 		interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cpg CPG_MOD 328>;
+
+		port {
+			usb3_peri0_ep: endpoint {
+				remote-endpoint = <&usb3_host0_ep>;
+			};
+		};
 	};
 
 	usb3_peri1: usb@ee060000 {
@@ -35,4 +44,10 @@ Example of R-Car H3 ES1.x:
 		reg = <0 0xee060000 0 0x400>;
 		interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cpg CPG_MOD 327>;
+
+		port {
+			usb3_peri1_ep: endpoint {
+				remote-endpoint = <&usb3_host1_ep>;
+			};
+		};
 	};
diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index 0875d38..7e4a5dd 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -193,6 +193,7 @@ config USB_RENESAS_USB3
 	tristate 'Renesas USB3.0 Peripheral controller'
 	depends on ARCH_RENESAS || COMPILE_TEST
 	depends on EXTCON && HAS_DMA
+	select USB_ROLE_SWITCH
 	help
 	   Renesas USB3.0 Peripheral controller is a USB peripheral controller
 	   that supports super, high, and full speed USB 3.0 data transfers.
diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
index 409cde4..c878449 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -23,6 +23,7 @@
 #include <linux/uaccess.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
+#include <linux/usb/role.h>
 
 /* register definitions */
 #define USB3_AXI_INT_STA	0x008
@@ -334,6 +335,9 @@ struct renesas_usb3 {
 	struct work_struct extcon_work;
 	struct phy *phy;
 
+	struct usb_role_switch *role_sw;
+	struct device *host_dev;
+
 	struct renesas_usb3_ep *usb3_ep;
 	int num_usb3_eps;
 
@@ -643,7 +647,7 @@ static void usb3_check_vbus(struct renesas_usb3 *usb3)
 	}
 }
 
-static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
+static void _usb3_set_mode(struct renesas_usb3 *usb3, bool host)
 {
 	if (host)
 		usb3_clear_bit(usb3, DRD_CON_PERI_CON, USB3_DRD_CON);
@@ -651,6 +655,11 @@ static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
 		usb3_set_bit(usb3, DRD_CON_PERI_CON, USB3_DRD_CON);
 }
 
+static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
+{
+	_usb3_set_mode(usb3, host);
+}
+
 static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
 {
 	if (enable)
@@ -2294,6 +2303,41 @@ static int renesas_usb3_set_selfpowered(struct usb_gadget *gadget, int is_self)
 	.set_selfpowered	= renesas_usb3_set_selfpowered,
 };
 
+static enum usb_role renesas_usb3_role_switch_get(struct device *dev)
+{
+	struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
+	enum usb_role cur_role;
+
+	pm_runtime_get_sync(dev);
+	cur_role = usb3_is_host(usb3) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
+	pm_runtime_put(dev);
+
+	return cur_role;
+}
+
+static int renesas_usb3_role_switch_set(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);
+	} else if (cur_role == USB_ROLE_DEVICE && role == USB_ROLE_HOST) {
+		/* Must set the mode before device_attach of the host */
+		_usb3_set_mode(usb3, true);
+		if (device_attach(host) < 0)
+			dev_err(dev, "device_attach(usb3_port) failed\n");
+	}
+	pm_runtime_put(dev);
+
+	return 0;
+}
+
 static ssize_t role_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
@@ -2404,6 +2448,8 @@ static int renesas_usb3_remove(struct platform_device *pdev)
 
 	device_remove_file(&pdev->dev, &dev_attr_role);
 
+	usb_role_switch_unregister(usb3->role_sw);
+
 	usb_del_gadget_udc(&usb3->gadget);
 	renesas_usb3_dma_free_prd(usb3, &pdev->dev);
 
@@ -2562,6 +2608,12 @@ static void renesas_usb3_init_ram(struct renesas_usb3 *usb3, struct device *dev,
 	EXTCON_NONE,
 };
 
+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 int renesas_usb3_probe(struct platform_device *pdev)
 {
 	struct renesas_usb3 *usb3;
@@ -2644,6 +2696,20 @@ static int renesas_usb3_probe(struct platform_device *pdev)
 	if (IS_ERR(usb3->phy))
 		usb3->phy = NULL;
 
+	usb3->role_sw = usb_role_switch_register(&pdev->dev,
+					&renesas_usb3_role_switch_desc);
+	if (!IS_ERR(usb3->role_sw)) {
+		usb3->host_dev = device_connection_find_by_graph(&pdev->dev,
+								 0, 0);
+		if (IS_ERR_OR_NULL(usb3->host_dev)) {
+			/* If not found, this driver will not use a role sw */
+			usb_role_switch_unregister(usb3->role_sw);
+			usb3->role_sw = NULL;
+		}
+	} else {
+		usb3->role_sw = NULL;
+	}
+
 	usb3->workaround_for_vbus = priv->workaround_for_vbus;
 
 	renesas_usb3_debugfs_init(usb3, &pdev->dev);

[PATCH/RFC v3 3/4] usb: gadget: udc: renesas_usb3: use usb role switch API

[Yoshihiro Shimoda]

This patch uses usb role switch API if the register suceeeded.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/usb/gadget/udc/renesas_usb3.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
index c878449..bfb5803 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -657,7 +657,11 @@ static void _usb3_set_mode(struct renesas_usb3 *usb3, bool host)
 
 static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
 {
-	_usb3_set_mode(usb3, host);
+	if (usb3->role_sw)
+		usb_role_switch_set_role(usb3->role_sw, host ?
+					 USB_ROLE_HOST : USB_ROLE_DEVICE);
+	else
+		_usb3_set_mode(usb3, host);
 }
 
 static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
@@ -672,8 +676,8 @@ 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(usb3, host);
+	spin_lock_irqsave(&usb3->lock, flags);
 	usb3_vbus_out(usb3, a_dev);
 	/* for A-Peripheral or forced B-device mode */
 	if ((!host && a_dev) ||
-- 
1.9.1

[PATCH/RFC,v3,3/4] usb: gadget: udc: renesas_usb3: use usb role switch API

[Yoshihiro Shimoda]

This patch uses usb role switch API if the register suceeeded.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/usb/gadget/udc/renesas_usb3.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
index c878449..bfb5803 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -657,7 +657,11 @@ static void _usb3_set_mode(struct renesas_usb3 *usb3, bool host)
 
 static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
 {
-	_usb3_set_mode(usb3, host);
+	if (usb3->role_sw)
+		usb_role_switch_set_role(usb3->role_sw, host ?
+					 USB_ROLE_HOST : USB_ROLE_DEVICE);
+	else
+		_usb3_set_mode(usb3, host);
 }
 
 static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
@@ -672,8 +676,8 @@ 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(usb3, host);
+	spin_lock_irqsave(&usb3->lock, flags);
 	usb3_vbus_out(usb3, a_dev);
 	/* for A-Peripheral or forced B-device mode */
 	if ((!host && a_dev) ||

[PATCH/RFC v3 4/4] arm64: dts: renesas: r8a7795: add OF graph for usb role switch

[Yoshihiro Shimoda]

This patch adds OF graph properties for usb role switch in r8a7795
into USB3.0 host/peripheral nodes.

TODO:
 - need patches for other SoCs.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 1d5e3ac..50d3312 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -1746,6 +1746,12 @@
 			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
 			resets = <&cpg 328>;
 			status = "disabled";
+
+			port {
+				usb3_host0_ep: endpoint {
+					remote-endpoint = <&usb3_peri0_ep>;
+				};
+			};
 		};
 
 		usb3_peri0: usb@ee020000 {
@@ -1757,6 +1763,12 @@
 			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
 			resets = <&cpg 328>;
 			status = "disabled";
+
+			port {
+				usb3_peri0_ep: endpoint {
+					remote-endpoint = <&usb3_host0_ep>;
+				};
+			};
 		};
 
 		usb_dmac0: dma-controller@e65a0000 {
-- 
1.9.1

[PATCH/RFC,v3,4/4] arm64: dts: renesas: r8a7795: add OF graph for usb role switch

[Yoshihiro Shimoda]

This patch adds OF graph properties for usb role switch in r8a7795
into USB3.0 host/peripheral nodes.

TODO:
 - need patches for other SoCs.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 1d5e3ac..50d3312 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -1746,6 +1746,12 @@
 			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
 			resets = <&cpg 328>;
 			status = "disabled";
+
+			port {
+				usb3_host0_ep: endpoint {
+					remote-endpoint = <&usb3_peri0_ep>;
+				};
+			};
 		};
 
 		usb3_peri0: usb@ee020000 {
@@ -1757,6 +1763,12 @@
 			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
 			resets = <&cpg 328>;
 			status = "disabled";
+
+			port {
+				usb3_peri0_ep: endpoint {
+					remote-endpoint = <&usb3_host0_ep>;
+				};
+			};
 		};
 
 		usb_dmac0: dma-controller@e65a0000 {

Re: [PATCH/RFC v3 1/4] base: devcon: add a new API to find the graph

[Heikki Krogerus]

On Mon, May 14, 2018 at 06:15:57PM +0900, Yoshihiro Shimoda wrote:
> This patch adds a new API "device_connection_find_by_graph()" to
> find device connection by using graph.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  Documentation/driver-api/device_connection.rst |  4 +--
>  drivers/base/devcon.c                          | 43 ++++++++++++++++++++++++++
>  include/linux/device.h                         |  2 ++
>  3 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/driver-api/device_connection.rst b/Documentation/driver-api/device_connection.rst
> index affbc556..2e2d26f 100644
> --- a/Documentation/driver-api/device_connection.rst
> +++ b/Documentation/driver-api/device_connection.rst
> @@ -19,7 +19,7 @@ Device connections alone do not create a dependency between the two devices.
>  They are only descriptions which are not tied to either of the devices directly.
>  A dependency between the two devices exists only if one of the two endpoint
>  devices requests a reference to the other. The descriptions themselves can be
> -defined in firmware (not yet supported) or they can be built-in.
> +defined in firmware or they can be built-in.
>  
>  Usage
>  -----
> @@ -40,4 +40,4 @@ API
>  ---
>  
>  .. kernel-doc:: drivers/base/devcon.c
> -   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove
> +   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove device_connection_find_by_graph
> diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
> index d427e80..5a0da33 100644
> --- a/drivers/base/devcon.c
> +++ b/drivers/base/devcon.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/property.h>
>  
>  static DEFINE_MUTEX(devcon_lock);
>  static LIST_HEAD(devcon_list);
> @@ -134,3 +135,45 @@ void device_connection_remove(struct device_connection *con)
>  	mutex_unlock(&devcon_lock);
>  }
>  EXPORT_SYMBOL_GPL(device_connection_remove);
> +
> +static int generic_graph_match(struct device *dev, void *fwnode)
> +{
> +	return dev->fwnode == fwnode;
> +}
> +
> +/**
> + * device_connection_find_by_graph - Find two devices connected together
> + * @dev: Device to find connected device
> + * @port: identifier of the @dev port node
> + * @endpoint: identifier of the @dev endpoint node
> + *
> + * Find a connection with @port and @endpoint by using graph between @dev and
> + * another device. On success returns handle to the device that is connected
> + * to @dev, with the reference count for the found device incremented. Returns
> + * NULL if no matching connection was found, or ERR_PTR(-EPROBE_DEFER) when
> + * a connection was found but the other device has not been enumerated yet.
> + */
> +struct device *device_connection_find_by_graph(struct device *dev, u32 port,
> +					       u32 endpoint)
> +{
> +	struct bus_type *bus;
> +	struct fwnode_handle *remote;
> +	struct device *conn;
> +
> +	remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> +	if (!remote)
> +		return NULL;
> +
> +	for (bus = generic_match_buses[0]; bus; bus++) {
> +		conn = bus_find_device(bus, NULL, remote, generic_graph_match);
> +		if (conn)
> +			return conn;
> +	}
> +
> +	/*
> +	 * We only get called if a connection was found, tell the caller to
> +	 * wait for the other device to show up.
> +	 */
> +	return ERR_PTR(-EPROBE_DEFER);
> +}
> +EXPORT_SYMBOL_GPL(device_connection_find_by_graph);

Why do we need more API for walking through the graph?

I'm not sure exactly sure what is going on here, I'll try to study
your patches more when I have time, but the approach looks wrong. That
function looks like a helper, but just not that useful one.

We really should be able to use the existing functions. In practice
device_connection_find_match() should eventually parse the graph, then
fallback to build-in connections if no graph is found. Otherwise
parsing graph here is not really useful at all.


Thanks,

-- 
heikki

[PATCH/RFC,v3,1/4] base: devcon: add a new API to find the graph

[Heikki Krogerus]

On Mon, May 14, 2018 at 06:15:57PM +0900, Yoshihiro Shimoda wrote:
> This patch adds a new API "device_connection_find_by_graph()" to
> find device connection by using graph.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  Documentation/driver-api/device_connection.rst |  4 +--
>  drivers/base/devcon.c                          | 43 ++++++++++++++++++++++++++
>  include/linux/device.h                         |  2 ++
>  3 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/driver-api/device_connection.rst b/Documentation/driver-api/device_connection.rst
> index affbc556..2e2d26f 100644
> --- a/Documentation/driver-api/device_connection.rst
> +++ b/Documentation/driver-api/device_connection.rst
> @@ -19,7 +19,7 @@ Device connections alone do not create a dependency between the two devices.
>  They are only descriptions which are not tied to either of the devices directly.
>  A dependency between the two devices exists only if one of the two endpoint
>  devices requests a reference to the other. The descriptions themselves can be
> -defined in firmware (not yet supported) or they can be built-in.
> +defined in firmware or they can be built-in.
>  
>  Usage
>  -----
> @@ -40,4 +40,4 @@ API
>  ---
>  
>  .. kernel-doc:: drivers/base/devcon.c
> -   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove
> +   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove device_connection_find_by_graph
> diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
> index d427e80..5a0da33 100644
> --- a/drivers/base/devcon.c
> +++ b/drivers/base/devcon.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/property.h>
>  
>  static DEFINE_MUTEX(devcon_lock);
>  static LIST_HEAD(devcon_list);
> @@ -134,3 +135,45 @@ void device_connection_remove(struct device_connection *con)
>  	mutex_unlock(&devcon_lock);
>  }
>  EXPORT_SYMBOL_GPL(device_connection_remove);
> +
> +static int generic_graph_match(struct device *dev, void *fwnode)
> +{
> +	return dev->fwnode == fwnode;
> +}
> +
> +/**
> + * device_connection_find_by_graph - Find two devices connected together
> + * @dev: Device to find connected device
> + * @port: identifier of the @dev port node
> + * @endpoint: identifier of the @dev endpoint node
> + *
> + * Find a connection with @port and @endpoint by using graph between @dev and
> + * another device. On success returns handle to the device that is connected
> + * to @dev, with the reference count for the found device incremented. Returns
> + * NULL if no matching connection was found, or ERR_PTR(-EPROBE_DEFER) when
> + * a connection was found but the other device has not been enumerated yet.
> + */
> +struct device *device_connection_find_by_graph(struct device *dev, u32 port,
> +					       u32 endpoint)
> +{
> +	struct bus_type *bus;
> +	struct fwnode_handle *remote;
> +	struct device *conn;
> +
> +	remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> +	if (!remote)
> +		return NULL;
> +
> +	for (bus = generic_match_buses[0]; bus; bus++) {
> +		conn = bus_find_device(bus, NULL, remote, generic_graph_match);
> +		if (conn)
> +			return conn;
> +	}
> +
> +	/*
> +	 * We only get called if a connection was found, tell the caller to
> +	 * wait for the other device to show up.
> +	 */
> +	return ERR_PTR(-EPROBE_DEFER);
> +}
> +EXPORT_SYMBOL_GPL(device_connection_find_by_graph);

Why do we need more API for walking through the graph?

I'm not sure exactly sure what is going on here, I'll try to study
your patches more when I have time, but the approach looks wrong. That
function looks like a helper, but just not that useful one.

We really should be able to use the existing functions. In practice
device_connection_find_match() should eventually parse the graph, then
fallback to build-in connections if no graph is found. Otherwise
parsing graph here is not really useful at all.


Thanks,

Re: [PATCH/RFC v3 1/4] base: devcon: add a new API to find the graph

[Randy Dunlap]

On 05/14/2018 02:15 AM, Yoshihiro Shimoda wrote:
> This patch adds a new API "device_connection_find_by_graph()" to
> find device connection by using graph.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  Documentation/driver-api/device_connection.rst |  4 +--
>  drivers/base/devcon.c                          | 43 ++++++++++++++++++++++++++
>  include/linux/device.h                         |  2 ++
>  3 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/driver-api/device_connection.rst b/Documentation/driver-api/device_connection.rst
> index affbc556..2e2d26f 100644
> --- a/Documentation/driver-api/device_connection.rst
> +++ b/Documentation/driver-api/device_connection.rst
> @@ -19,7 +19,7 @@ Device connections alone do not create a dependency between the two devices.
>  They are only descriptions which are not tied to either of the devices directly.
>  A dependency between the two devices exists only if one of the two endpoint
>  devices requests a reference to the other. The descriptions themselves can be
> -defined in firmware (not yet supported) or they can be built-in.
> +defined in firmware or they can be built-in.
>  
>  Usage
>  -----
> @@ -40,4 +40,4 @@ API
>  ---
>  
>  .. kernel-doc:: drivers/base/devcon.c
> -   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove
> +   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove device_connection_find_by_graph

BTW, that line above should be like:
      :functions: ...
i.e., no space after the first ':'.

I have sent a patch for that and Jon Corbet has applied it to his docs tree.

-- 
~Randy

[PATCH/RFC,v3,1/4] base: devcon: add a new API to find the graph

[Randy Dunlap]

On 05/14/2018 02:15 AM, Yoshihiro Shimoda wrote:
> This patch adds a new API "device_connection_find_by_graph()" to
> find device connection by using graph.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  Documentation/driver-api/device_connection.rst |  4 +--
>  drivers/base/devcon.c                          | 43 ++++++++++++++++++++++++++
>  include/linux/device.h                         |  2 ++
>  3 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/driver-api/device_connection.rst b/Documentation/driver-api/device_connection.rst
> index affbc556..2e2d26f 100644
> --- a/Documentation/driver-api/device_connection.rst
> +++ b/Documentation/driver-api/device_connection.rst
> @@ -19,7 +19,7 @@ Device connections alone do not create a dependency between the two devices.
>  They are only descriptions which are not tied to either of the devices directly.
>  A dependency between the two devices exists only if one of the two endpoint
>  devices requests a reference to the other. The descriptions themselves can be
> -defined in firmware (not yet supported) or they can be built-in.
> +defined in firmware or they can be built-in.
>  
>  Usage
>  -----
> @@ -40,4 +40,4 @@ API
>  ---
>  
>  .. kernel-doc:: drivers/base/devcon.c
> -   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove
> +   : functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove device_connection_find_by_graph

BTW, that line above should be like:
      :functions: ...
i.e., no space after the first ':'.

I have sent a patch for that and Jon Corbet has applied it to his docs tree.

RE: [PATCH/RFC v3 1/4] base: devcon: add a new API to find the graph

[Yoshihiro Shimoda]

Hi Heikki,

> From: Heikki Krogerus, Sent: Monday, May 14, 2018 10:28 PM
> 
> On Mon, May 14, 2018 at 06:15:57PM +0900, Yoshihiro Shimoda wrote:
<snip>
> > diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
> > index d427e80..5a0da33 100644
> > --- a/drivers/base/devcon.c
> > +++ b/drivers/base/devcon.c
<snip>
> > +static int generic_graph_match(struct device *dev, void *fwnode)
> > +{
> > +	return dev->fwnode == fwnode;
> > +}
> > +
> > +/**
> > + * device_connection_find_by_graph - Find two devices connected together
> > + * @dev: Device to find connected device
> > + * @port: identifier of the @dev port node
> > + * @endpoint: identifier of the @dev endpoint node
> > + *
> > + * Find a connection with @port and @endpoint by using graph between @dev and
> > + * another device. On success returns handle to the device that is connected
> > + * to @dev, with the reference count for the found device incremented. Returns
> > + * NULL if no matching connection was found, or ERR_PTR(-EPROBE_DEFER) when
> > + * a connection was found but the other device has not been enumerated yet.
> > + */
> > +struct device *device_connection_find_by_graph(struct device *dev, u32 port,
> > +					       u32 endpoint)
> > +{
> > +	struct bus_type *bus;
> > +	struct fwnode_handle *remote;
> > +	struct device *conn;
> > +
> > +	remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> > +	if (!remote)
> > +		return NULL;
> > +
> > +	for (bus = generic_match_buses[0]; bus; bus++) {
> > +		conn = bus_find_device(bus, NULL, remote, generic_graph_match);
> > +		if (conn)
> > +			return conn;
> > +	}
> > +
> > +	/*
> > +	 * We only get called if a connection was found, tell the caller to
> > +	 * wait for the other device to show up.
> > +	 */
> > +	return ERR_PTR(-EPROBE_DEFER);
> > +}
> > +EXPORT_SYMBOL_GPL(device_connection_find_by_graph);
> 
> Why do we need more API for walking through the graph?

I thought there is difficult to find if a device has multiple ports or endpoints.
So, I'd like to use port and endpoint number for finding a device.

> I'm not sure exactly sure what is going on here, I'll try to study
> your patches more when I have time, but the approach looks wrong. That
> function looks like a helper, but just not that useful one.
> 
> We really should be able to use the existing functions. In practice
> device_connection_find_match() should eventually parse the graph, then
> fallback to build-in connections if no graph is found. Otherwise
> parsing graph here is not really useful at all.

I think using device_connection_find_match() for finding the graph becomes complicated.
The current arguments of the function is the below:

void *device_connection_find_match(struct device *dev, const char *con_id,
			       void *data,
			       void *(*match)(struct device_connection *con,
					      int ep, void *data))

If finding the graph, the following arguments will be not used.
 - con_id
 - *con and ep of "match" arguments.

This is because these arguments are for the build-in connections.
So, should I modify the arguments of the function for finding both
the graph and built-in connections somehow?

Best regards,
Yoshihiro Shimoda

> 
> Thanks,
> 
> --
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC,v3,1/4] base: devcon: add a new API to find the graph

[Yoshihiro Shimoda]

Hi Heikki,

> From: Heikki Krogerus, Sent: Monday, May 14, 2018 10:28 PM
> 
> On Mon, May 14, 2018 at 06:15:57PM +0900, Yoshihiro Shimoda wrote:
<snip>
> > diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
> > index d427e80..5a0da33 100644
> > --- a/drivers/base/devcon.c
> > +++ b/drivers/base/devcon.c
<snip>
> > +static int generic_graph_match(struct device *dev, void *fwnode)
> > +{
> > +	return dev->fwnode == fwnode;
> > +}
> > +
> > +/**
> > + * device_connection_find_by_graph - Find two devices connected together
> > + * @dev: Device to find connected device
> > + * @port: identifier of the @dev port node
> > + * @endpoint: identifier of the @dev endpoint node
> > + *
> > + * Find a connection with @port and @endpoint by using graph between @dev and
> > + * another device. On success returns handle to the device that is connected
> > + * to @dev, with the reference count for the found device incremented. Returns
> > + * NULL if no matching connection was found, or ERR_PTR(-EPROBE_DEFER) when
> > + * a connection was found but the other device has not been enumerated yet.
> > + */
> > +struct device *device_connection_find_by_graph(struct device *dev, u32 port,
> > +					       u32 endpoint)
> > +{
> > +	struct bus_type *bus;
> > +	struct fwnode_handle *remote;
> > +	struct device *conn;
> > +
> > +	remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> > +	if (!remote)
> > +		return NULL;
> > +
> > +	for (bus = generic_match_buses[0]; bus; bus++) {
> > +		conn = bus_find_device(bus, NULL, remote, generic_graph_match);
> > +		if (conn)
> > +			return conn;
> > +	}
> > +
> > +	/*
> > +	 * We only get called if a connection was found, tell the caller to
> > +	 * wait for the other device to show up.
> > +	 */
> > +	return ERR_PTR(-EPROBE_DEFER);
> > +}
> > +EXPORT_SYMBOL_GPL(device_connection_find_by_graph);
> 
> Why do we need more API for walking through the graph?

I thought there is difficult to find if a device has multiple ports or endpoints.
So, I'd like to use port and endpoint number for finding a device.

> I'm not sure exactly sure what is going on here, I'll try to study
> your patches more when I have time, but the approach looks wrong. That
> function looks like a helper, but just not that useful one.
> 
> We really should be able to use the existing functions. In practice
> device_connection_find_match() should eventually parse the graph, then
> fallback to build-in connections if no graph is found. Otherwise
> parsing graph here is not really useful at all.

I think using device_connection_find_match() for finding the graph becomes complicated.
The current arguments of the function is the below:

void *device_connection_find_match(struct device *dev, const char *con_id,
			       void *data,
			       void *(*match)(struct device_connection *con,
					      int ep, void *data))

If finding the graph, the following arguments will be not used.
 - con_id
 - *con and ep of "match" arguments.

This is because these arguments are for the build-in connections.
So, should I modify the arguments of the function for finding both
the graph and built-in connections somehow?

Best regards,
Yoshihiro Shimoda

> 
> Thanks,
> 
> --
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC v3 3/4] usb: gadget: udc: renesas_usb3: use usb role switch API

[Simon Horman]

On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote:
> This patch uses usb role switch API if the register suceeeded.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/usb/gadget/udc/renesas_usb3.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
> index c878449..bfb5803 100644
> --- a/drivers/usb/gadget/udc/renesas_usb3.c
> +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> @@ -657,7 +657,11 @@ static void _usb3_set_mode(struct renesas_usb3 *usb3, bool host)
>  
>  static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
>  {
> -	_usb3_set_mode(usb3, host);
> +	if (usb3->role_sw)
> +		usb_role_switch_set_role(usb3->role_sw, host ?
> +					 USB_ROLE_HOST : USB_ROLE_DEVICE);
> +	else
> +		_usb3_set_mode(usb3, host);
>  }
>  
>  static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
> @@ -672,8 +676,8 @@ 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(usb3, host);
> +	spin_lock_irqsave(&usb3->lock, flags);

Hi Shimoda-san,

could you clarify why moving this lock is safe?

>  	usb3_vbus_out(usb3, a_dev);
>  	/* for A-Peripheral or forced B-device mode */
>  	if ((!host && a_dev) ||
> -- 
> 1.9.1
> 

[PATCH/RFC,v3,3/4] usb: gadget: udc: renesas_usb3: use usb role switch API

[Simon Horman]

On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote:
> This patch uses usb role switch API if the register suceeeded.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/usb/gadget/udc/renesas_usb3.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
> index c878449..bfb5803 100644
> --- a/drivers/usb/gadget/udc/renesas_usb3.c
> +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> @@ -657,7 +657,11 @@ static void _usb3_set_mode(struct renesas_usb3 *usb3, bool host)
>  
>  static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
>  {
> -	_usb3_set_mode(usb3, host);
> +	if (usb3->role_sw)
> +		usb_role_switch_set_role(usb3->role_sw, host ?
> +					 USB_ROLE_HOST : USB_ROLE_DEVICE);
> +	else
> +		_usb3_set_mode(usb3, host);
>  }
>  
>  static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
> @@ -672,8 +676,8 @@ 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(usb3, host);
> +	spin_lock_irqsave(&usb3->lock, flags);

Hi Shimoda-san,

could you clarify why moving this lock is safe?

>  	usb3_vbus_out(usb3, a_dev);
>  	/* for A-Peripheral or forced B-device mode */
>  	if ((!host && a_dev) ||
> -- 
> 1.9.1
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH/RFC v3 3/4] usb: gadget: udc: renesas_usb3: use usb role switch API

[Yoshihiro Shimoda]

Hi Simon-san,

Thank you for your review!

> From: Simon Horman, Sent: Tuesday, May 15, 2018 4:54 PM
> On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote:
<snip>
> >  static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
> >  {
> > -	_usb3_set_mode(usb3, host);
> > +	if (usb3->role_sw)
> > +		usb_role_switch_set_role(usb3->role_sw, host ?
> > +					 USB_ROLE_HOST : USB_ROLE_DEVICE);
> > +	else
> > +		_usb3_set_mode(usb3, host);
> >  }
> >
> >  static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
> > @@ -672,8 +676,8 @@ 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(usb3, host);
> > +	spin_lock_irqsave(&usb3->lock, flags);
> 
> Hi Shimoda-san,
> 
> could you clarify why moving this lock is safe?

The usb3_set_mode() is possible to call usb_role_switch_set_role() API
and usb_role_switch_set_role() calls mutex_lock().
So, this moving this lock (especially avoiding irqsave) needs.

Best regards,
Yoshihiro Shimoda

> >  	usb3_vbus_out(usb3, a_dev);
> >  	/* for A-Peripheral or forced B-device mode */
> >  	if ((!host && a_dev) ||
> > --
> > 1.9.1
> >

[PATCH/RFC,v3,3/4] usb: gadget: udc: renesas_usb3: use usb role switch API

[Yoshihiro Shimoda]

Hi Simon-san,

Thank you for your review!

> From: Simon Horman, Sent: Tuesday, May 15, 2018 4:54 PM
> On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote:
<snip>
> >  static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
> >  {
> > -	_usb3_set_mode(usb3, host);
> > +	if (usb3->role_sw)
> > +		usb_role_switch_set_role(usb3->role_sw, host ?
> > +					 USB_ROLE_HOST : USB_ROLE_DEVICE);
> > +	else
> > +		_usb3_set_mode(usb3, host);
> >  }
> >
> >  static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
> > @@ -672,8 +676,8 @@ 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(usb3, host);
> > +	spin_lock_irqsave(&usb3->lock, flags);
> 
> Hi Shimoda-san,
> 
> could you clarify why moving this lock is safe?

The usb3_set_mode() is possible to call usb_role_switch_set_role() API
and usb_role_switch_set_role() calls mutex_lock().
So, this moving this lock (especially avoiding irqsave) needs.

Best regards,
Yoshihiro Shimoda

> >  	usb3_vbus_out(usb3, a_dev);
> >  	/* for A-Peripheral or forced B-device mode */
> >  	if ((!host && a_dev) ||
> > --
> > 1.9.1
> >
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC v3 3/4] usb: gadget: udc: renesas_usb3: use usb role switch API

[Simon Horman]

On Tue, May 15, 2018 at 08:02:00AM +0000, Yoshihiro Shimoda wrote:
> Hi Simon-san,
> 
> Thank you for your review!
> 
> > From: Simon Horman, Sent: Tuesday, May 15, 2018 4:54 PM
> > On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote:
> <snip>
> > >  static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
> > >  {
> > > -	_usb3_set_mode(usb3, host);
> > > +	if (usb3->role_sw)
> > > +		usb_role_switch_set_role(usb3->role_sw, host ?
> > > +					 USB_ROLE_HOST : USB_ROLE_DEVICE);
> > > +	else
> > > +		_usb3_set_mode(usb3, host);
> > >  }
> > >
> > >  static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
> > > @@ -672,8 +676,8 @@ 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(usb3, host);
> > > +	spin_lock_irqsave(&usb3->lock, flags);
> > 
> > Hi Shimoda-san,
> > 
> > could you clarify why moving this lock is safe?
> 
> The usb3_set_mode() is possible to call usb_role_switch_set_role() API
> and usb_role_switch_set_role() calls mutex_lock().
> So, this moving this lock (especially avoiding irqsave) needs.

Thanks, that make sense.

But usb3_set_mode() may also call _usb3_set_mode().
It is safe to call _usb3_set_mode() without holding the lock?

> Best regards,
> Yoshihiro Shimoda
> 
> > >  	usb3_vbus_out(usb3, a_dev);
> > >  	/* for A-Peripheral or forced B-device mode */
> > >  	if ((!host && a_dev) ||
> > > --
> > > 1.9.1
> > >
> 

[PATCH/RFC,v3,3/4] usb: gadget: udc: renesas_usb3: use usb role switch API

[Simon Horman]

On Tue, May 15, 2018 at 08:02:00AM +0000, Yoshihiro Shimoda wrote:
> Hi Simon-san,
> 
> Thank you for your review!
> 
> > From: Simon Horman, Sent: Tuesday, May 15, 2018 4:54 PM
> > On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote:
> <snip>
> > >  static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
> > >  {
> > > -	_usb3_set_mode(usb3, host);
> > > +	if (usb3->role_sw)
> > > +		usb_role_switch_set_role(usb3->role_sw, host ?
> > > +					 USB_ROLE_HOST : USB_ROLE_DEVICE);
> > > +	else
> > > +		_usb3_set_mode(usb3, host);
> > >  }
> > >
> > >  static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
> > > @@ -672,8 +676,8 @@ 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(usb3, host);
> > > +	spin_lock_irqsave(&usb3->lock, flags);
> > 
> > Hi Shimoda-san,
> > 
> > could you clarify why moving this lock is safe?
> 
> The usb3_set_mode() is possible to call usb_role_switch_set_role() API
> and usb_role_switch_set_role() calls mutex_lock().
> So, this moving this lock (especially avoiding irqsave) needs.

Thanks, that make sense.

But usb3_set_mode() may also call _usb3_set_mode().
It is safe to call _usb3_set_mode() without holding the lock?

> Best regards,
> Yoshihiro Shimoda
> 
> > >  	usb3_vbus_out(usb3, a_dev);
> > >  	/* for A-Peripheral or forced B-device mode */
> > >  	if ((!host && a_dev) ||
> > > --
> > > 1.9.1
> > >
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC v3 1/4] base: devcon: add a new API to find the graph

[Heikki Krogerus]

On Tue, May 15, 2018 at 02:19:14AM +0000, Yoshihiro Shimoda wrote:
> Hi Heikki,
> 
> > From: Heikki Krogerus, Sent: Monday, May 14, 2018 10:28 PM
> > 
> > On Mon, May 14, 2018 at 06:15:57PM +0900, Yoshihiro Shimoda wrote:
> <snip>
> > > diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
> > > index d427e80..5a0da33 100644
> > > --- a/drivers/base/devcon.c
> > > +++ b/drivers/base/devcon.c
> <snip>
> > > +static int generic_graph_match(struct device *dev, void *fwnode)
> > > +{
> > > +	return dev->fwnode == fwnode;
> > > +}
> > > +
> > > +/**
> > > + * device_connection_find_by_graph - Find two devices connected together
> > > + * @dev: Device to find connected device
> > > + * @port: identifier of the @dev port node
> > > + * @endpoint: identifier of the @dev endpoint node
> > > + *
> > > + * Find a connection with @port and @endpoint by using graph between @dev and
> > > + * another device. On success returns handle to the device that is connected
> > > + * to @dev, with the reference count for the found device incremented. Returns
> > > + * NULL if no matching connection was found, or ERR_PTR(-EPROBE_DEFER) when
> > > + * a connection was found but the other device has not been enumerated yet.
> > > + */
> > > +struct device *device_connection_find_by_graph(struct device *dev, u32 port,
> > > +					       u32 endpoint)
> > > +{
> > > +	struct bus_type *bus;
> > > +	struct fwnode_handle *remote;
> > > +	struct device *conn;
> > > +
> > > +	remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> > > +	if (!remote)
> > > +		return NULL;
> > > +
> > > +	for (bus = generic_match_buses[0]; bus; bus++) {
> > > +		conn = bus_find_device(bus, NULL, remote, generic_graph_match);
> > > +		if (conn)
> > > +			return conn;
> > > +	}
> > > +
> > > +	/*
> > > +	 * We only get called if a connection was found, tell the caller to
> > > +	 * wait for the other device to show up.
> > > +	 */
> > > +	return ERR_PTR(-EPROBE_DEFER);
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_connection_find_by_graph);
> > 
> > Why do we need more API for walking through the graph?
> 
> I thought there is difficult to find if a device has multiple ports or endpoints.
> So, I'd like to use port and endpoint number for finding a device.
> 
> > I'm not sure exactly sure what is going on here, I'll try to study
> > your patches more when I have time, but the approach looks wrong. That
> > function looks like a helper, but just not that useful one.
> > 
> > We really should be able to use the existing functions. In practice
> > device_connection_find_match() should eventually parse the graph, then
> > fallback to build-in connections if no graph is found. Otherwise
> > parsing graph here is not really useful at all.
> 
> I think using device_connection_find_match() for finding the graph becomes complicated.
> The current arguments of the function is the below:
> 
> void *device_connection_find_match(struct device *dev, const char *con_id,
> 			       void *data,
> 			       void *(*match)(struct device_connection *con,
> 					      int ep, void *data))
> 
> If finding the graph, the following arguments will be not used.
>  - con_id
>  - *con and ep of "match" arguments.
> 
> This is because these arguments are for the build-in connections.

No they're not. You do realize that we can build a temporary struct
device_connection there and then (in stack) to be supplied for the
->match callback.

> So, should I modify the arguments of the function for finding both
> the graph and built-in connections somehow?

I don't see any need for that. We may need to modify struct
device_connection, but there should not be any need to touch the API.

Your plan to use port and endpoint number is wrong, as they are just
indexes, and therefore not reliable. Luckily it should be completely
unnecessary to use them.

The way I see this working is that we parse all the endpoints the
caller device has. If we can't take advantage of the con_id for
identification (though ideally we can), we just need to call ->match
with every one of them. The implementation for the ->match callback is
then responsible of figuring out if the endpoint is the one we are
looking for or not in that case.

The only problem I see with that is that we may not have a reliable
way to convert the fwnode pointing to the remote-endpoint parent into
struct device (if one has already been enumerated) in order to get the
device name and use it as the second endpoint name in struct
device_connection. To solve that, we could consider for example just
adding a new member, fwnode pointer perhaps, to the structure. Or
perhaps use the endpoint members for something else than device names
when graph is used, and add a member defining the type of match:
graph, build-in, etc. There are many things we can consider.

I don't know if I'm able to explain what I'm after with this, so if
you like, I can propose something for this myself. Though that will
have to wait for few weeks. Right now I'm too busy with other stuff.


Thanks,

-- 
heikki

[PATCH/RFC,v3,1/4] base: devcon: add a new API to find the graph

[Heikki Krogerus]

On Tue, May 15, 2018 at 02:19:14AM +0000, Yoshihiro Shimoda wrote:
> Hi Heikki,
> 
> > From: Heikki Krogerus, Sent: Monday, May 14, 2018 10:28 PM
> > 
> > On Mon, May 14, 2018 at 06:15:57PM +0900, Yoshihiro Shimoda wrote:
> <snip>
> > > diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
> > > index d427e80..5a0da33 100644
> > > --- a/drivers/base/devcon.c
> > > +++ b/drivers/base/devcon.c
> <snip>
> > > +static int generic_graph_match(struct device *dev, void *fwnode)
> > > +{
> > > +	return dev->fwnode == fwnode;
> > > +}
> > > +
> > > +/**
> > > + * device_connection_find_by_graph - Find two devices connected together
> > > + * @dev: Device to find connected device
> > > + * @port: identifier of the @dev port node
> > > + * @endpoint: identifier of the @dev endpoint node
> > > + *
> > > + * Find a connection with @port and @endpoint by using graph between @dev and
> > > + * another device. On success returns handle to the device that is connected
> > > + * to @dev, with the reference count for the found device incremented. Returns
> > > + * NULL if no matching connection was found, or ERR_PTR(-EPROBE_DEFER) when
> > > + * a connection was found but the other device has not been enumerated yet.
> > > + */
> > > +struct device *device_connection_find_by_graph(struct device *dev, u32 port,
> > > +					       u32 endpoint)
> > > +{
> > > +	struct bus_type *bus;
> > > +	struct fwnode_handle *remote;
> > > +	struct device *conn;
> > > +
> > > +	remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> > > +	if (!remote)
> > > +		return NULL;
> > > +
> > > +	for (bus = generic_match_buses[0]; bus; bus++) {
> > > +		conn = bus_find_device(bus, NULL, remote, generic_graph_match);
> > > +		if (conn)
> > > +			return conn;
> > > +	}
> > > +
> > > +	/*
> > > +	 * We only get called if a connection was found, tell the caller to
> > > +	 * wait for the other device to show up.
> > > +	 */
> > > +	return ERR_PTR(-EPROBE_DEFER);
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_connection_find_by_graph);
> > 
> > Why do we need more API for walking through the graph?
> 
> I thought there is difficult to find if a device has multiple ports or endpoints.
> So, I'd like to use port and endpoint number for finding a device.
> 
> > I'm not sure exactly sure what is going on here, I'll try to study
> > your patches more when I have time, but the approach looks wrong. That
> > function looks like a helper, but just not that useful one.
> > 
> > We really should be able to use the existing functions. In practice
> > device_connection_find_match() should eventually parse the graph, then
> > fallback to build-in connections if no graph is found. Otherwise
> > parsing graph here is not really useful at all.
> 
> I think using device_connection_find_match() for finding the graph becomes complicated.
> The current arguments of the function is the below:
> 
> void *device_connection_find_match(struct device *dev, const char *con_id,
> 			       void *data,
> 			       void *(*match)(struct device_connection *con,
> 					      int ep, void *data))
> 
> If finding the graph, the following arguments will be not used.
>  - con_id
>  - *con and ep of "match" arguments.
> 
> This is because these arguments are for the build-in connections.

No they're not. You do realize that we can build a temporary struct
device_connection there and then (in stack) to be supplied for the
->match callback.

> So, should I modify the arguments of the function for finding both
> the graph and built-in connections somehow?

I don't see any need for that. We may need to modify struct
device_connection, but there should not be any need to touch the API.

Your plan to use port and endpoint number is wrong, as they are just
indexes, and therefore not reliable. Luckily it should be completely
unnecessary to use them.

The way I see this working is that we parse all the endpoints the
caller device has. If we can't take advantage of the con_id for
identification (though ideally we can), we just need to call ->match
with every one of them. The implementation for the ->match callback is
then responsible of figuring out if the endpoint is the one we are
looking for or not in that case.

The only problem I see with that is that we may not have a reliable
way to convert the fwnode pointing to the remote-endpoint parent into
struct device (if one has already been enumerated) in order to get the
device name and use it as the second endpoint name in struct
device_connection. To solve that, we could consider for example just
adding a new member, fwnode pointer perhaps, to the structure. Or
perhaps use the endpoint members for something else than device names
when graph is used, and add a member defining the type of match:
graph, build-in, etc. There are many things we can consider.

I don't know if I'm able to explain what I'm after with this, so if
you like, I can propose something for this myself. Though that will
have to wait for few weeks. Right now I'm too busy with other stuff.


Thanks,

RE: [PATCH/RFC v3 3/4] usb: gadget: udc: renesas_usb3: use usb role switch API

[Yoshihiro Shimoda]

Hi Simon-san,

> From: Simon Horman, Sent: Tuesday, May 15, 2018 5:04 PM
> 
> On Tue, May 15, 2018 at 08:02:00AM +0000, Yoshihiro Shimoda wrote:
> > Hi Simon-san,
> >
> > Thank you for your review!
> >
> > > From: Simon Horman, Sent: Tuesday, May 15, 2018 4:54 PM
> > > On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote:
> > <snip>
> > > >  static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
> > > >  {
> > > > -	_usb3_set_mode(usb3, host);
> > > > +	if (usb3->role_sw)
> > > > +		usb_role_switch_set_role(usb3->role_sw, host ?
> > > > +					 USB_ROLE_HOST : USB_ROLE_DEVICE);
> > > > +	else
> > > > +		_usb3_set_mode(usb3, host);
> > > >  }
> > > >
> > > >  static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
> > > > @@ -672,8 +676,8 @@ 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(usb3, host);
> > > > +	spin_lock_irqsave(&usb3->lock, flags);
> > >
> > > Hi Shimoda-san,
> > >
> > > could you clarify why moving this lock is safe?
> >
> > The usb3_set_mode() is possible to call usb_role_switch_set_role() API
> > and usb_role_switch_set_role() calls mutex_lock().
> > So, this moving this lock (especially avoiding irqsave) needs.
> 
> Thanks, that make sense.
> 
> But usb3_set_mode() may also call _usb3_set_mode().
> It is safe to call _usb3_set_mode() without holding the lock?

Thank you for the pointed out. I am thinking it is possible to be
unsafe without holding the lock. So, I'd like to improve this patch somehow
(for example: Add new usb role switch APIs without mutex).

Best regards,
Yoshihiro Shimoda

[PATCH/RFC,v3,3/4] usb: gadget: udc: renesas_usb3: use usb role switch API

[Yoshihiro Shimoda]

Hi Simon-san,

> From: Simon Horman, Sent: Tuesday, May 15, 2018 5:04 PM
> 
> On Tue, May 15, 2018 at 08:02:00AM +0000, Yoshihiro Shimoda wrote:
> > Hi Simon-san,
> >
> > Thank you for your review!
> >
> > > From: Simon Horman, Sent: Tuesday, May 15, 2018 4:54 PM
> > > On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote:
> > <snip>
> > > >  static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
> > > >  {
> > > > -	_usb3_set_mode(usb3, host);
> > > > +	if (usb3->role_sw)
> > > > +		usb_role_switch_set_role(usb3->role_sw, host ?
> > > > +					 USB_ROLE_HOST : USB_ROLE_DEVICE);
> > > > +	else
> > > > +		_usb3_set_mode(usb3, host);
> > > >  }
> > > >
> > > >  static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable)
> > > > @@ -672,8 +676,8 @@ 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(usb3, host);
> > > > +	spin_lock_irqsave(&usb3->lock, flags);
> > >
> > > Hi Shimoda-san,
> > >
> > > could you clarify why moving this lock is safe?
> >
> > The usb3_set_mode() is possible to call usb_role_switch_set_role() API
> > and usb_role_switch_set_role() calls mutex_lock().
> > So, this moving this lock (especially avoiding irqsave) needs.
> 
> Thanks, that make sense.
> 
> But usb3_set_mode() may also call _usb3_set_mode().
> It is safe to call _usb3_set_mode() without holding the lock?

Thank you for the pointed out. I am thinking it is possible to be
unsafe without holding the lock. So, I'd like to improve this patch somehow
(for example: Add new usb role switch APIs without mutex).

Best regards,
Yoshihiro Shimoda
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH/RFC v3 1/4] base: devcon: add a new API to find the graph

[Yoshihiro Shimoda]

Hi Heikki,

Thank you for the reply!

> From: Heikki Krogerus, Sent: Tuesday, May 15, 2018 5:29 PM
> 
> On Tue, May 15, 2018 at 02:19:14AM +0000, Yoshihiro Shimoda wrote:
> > Hi Heikki,
> >
> > > From: Heikki Krogerus, Sent: Monday, May 14, 2018 10:28 PM
> > >
> > > On Mon, May 14, 2018 at 06:15:57PM +0900, Yoshihiro Shimoda wrote:
<snip>
> > > > +struct device *device_connection_find_by_graph(struct device *dev, u32 port,
> > > > +					       u32 endpoint)
> > > > +{
> > > > +	struct bus_type *bus;
> > > > +	struct fwnode_handle *remote;
> > > > +	struct device *conn;
> > > > +
> > > > +	remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> > > > +	if (!remote)
> > > > +		return NULL;
> > > > +
> > > > +	for (bus = generic_match_buses[0]; bus; bus++) {
> > > > +		conn = bus_find_device(bus, NULL, remote, generic_graph_match);
> > > > +		if (conn)
> > > > +			return conn;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * We only get called if a connection was found, tell the caller to
> > > > +	 * wait for the other device to show up.
> > > > +	 */
> > > > +	return ERR_PTR(-EPROBE_DEFER);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(device_connection_find_by_graph);
> > >
> > > Why do we need more API for walking through the graph?
> >
> > I thought there is difficult to find if a device has multiple ports or endpoints.
> > So, I'd like to use port and endpoint number for finding a device.
> >
> > > I'm not sure exactly sure what is going on here, I'll try to study
> > > your patches more when I have time, but the approach looks wrong. That
> > > function looks like a helper, but just not that useful one.
> > >
> > > We really should be able to use the existing functions. In practice
> > > device_connection_find_match() should eventually parse the graph, then
> > > fallback to build-in connections if no graph is found. Otherwise
> > > parsing graph here is not really useful at all.
> >
> > I think using device_connection_find_match() for finding the graph becomes complicated.
> > The current arguments of the function is the below:
> >
> > void *device_connection_find_match(struct device *dev, const char *con_id,
> > 			       void *data,
> > 			       void *(*match)(struct device_connection *con,
> > 					      int ep, void *data))
> >
> > If finding the graph, the following arguments will be not used.
> >  - con_id
> >  - *con and ep of "match" arguments.
> >
> > This is because these arguments are for the build-in connections.
> 
> No they're not. You do realize that we can build a temporary struct
> device_connection there and then (in stack) to be supplied for the
> ->match callback.

I understood it.

> > So, should I modify the arguments of the function for finding both
> > the graph and built-in connections somehow?
> 
> I don't see any need for that. We may need to modify struct
> device_connection, but there should not be any need to touch the API.
> 
> Your plan to use port and endpoint number is wrong, as they are just
> indexes, and therefore not reliable. Luckily it should be completely
> unnecessary to use them.
> 
> The way I see this working is that we parse all the endpoints the
> caller device has. If we can't take advantage of the con_id for
> identification (though ideally we can), we just need to call ->match
> with every one of them. The implementation for the ->match callback is
> then responsible of figuring out if the endpoint is the one we are
> looking for or not in that case.

I understood it. But, I need to investigate how to find a device.

> The only problem I see with that is that we may not have a reliable
> way to convert the fwnode pointing to the remote-endpoint parent into
> struct device (if one has already been enumerated) in order to get the
> device name and use it as the second endpoint name in struct
> device_connection. To solve that, we could consider for example just
> adding a new member, fwnode pointer perhaps, to the structure. Or
> perhaps use the endpoint members for something else than device names
> when graph is used, and add a member defining the type of match:
> graph, build-in, etc. There are many things we can consider.

I don't understand why adding such new member(s) can solve that.
Anyway, I will investigate this more...

> I don't know if I'm able to explain what I'm after with this, so if
> you like, I can propose something for this myself. Though that will
> have to wait for few weeks. Right now I'm too busy with other stuff.

Thank you for your proposal! However, I'd like to try to investigate
once more while you are too busy.

Best regards,
Yoshihiro Shimoda

> 
> Thanks,
> 
> --
> heikki

[PATCH/RFC,v3,1/4] base: devcon: add a new API to find the graph

[Yoshihiro Shimoda]

Hi Heikki,

Thank you for the reply!

> From: Heikki Krogerus, Sent: Tuesday, May 15, 2018 5:29 PM
> 
> On Tue, May 15, 2018 at 02:19:14AM +0000, Yoshihiro Shimoda wrote:
> > Hi Heikki,
> >
> > > From: Heikki Krogerus, Sent: Monday, May 14, 2018 10:28 PM
> > >
> > > On Mon, May 14, 2018 at 06:15:57PM +0900, Yoshihiro Shimoda wrote:
<snip>
> > > > +struct device *device_connection_find_by_graph(struct device *dev, u32 port,
> > > > +					       u32 endpoint)
> > > > +{
> > > > +	struct bus_type *bus;
> > > > +	struct fwnode_handle *remote;
> > > > +	struct device *conn;
> > > > +
> > > > +	remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> > > > +	if (!remote)
> > > > +		return NULL;
> > > > +
> > > > +	for (bus = generic_match_buses[0]; bus; bus++) {
> > > > +		conn = bus_find_device(bus, NULL, remote, generic_graph_match);
> > > > +		if (conn)
> > > > +			return conn;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * We only get called if a connection was found, tell the caller to
> > > > +	 * wait for the other device to show up.
> > > > +	 */
> > > > +	return ERR_PTR(-EPROBE_DEFER);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(device_connection_find_by_graph);
> > >
> > > Why do we need more API for walking through the graph?
> >
> > I thought there is difficult to find if a device has multiple ports or endpoints.
> > So, I'd like to use port and endpoint number for finding a device.
> >
> > > I'm not sure exactly sure what is going on here, I'll try to study
> > > your patches more when I have time, but the approach looks wrong. That
> > > function looks like a helper, but just not that useful one.
> > >
> > > We really should be able to use the existing functions. In practice
> > > device_connection_find_match() should eventually parse the graph, then
> > > fallback to build-in connections if no graph is found. Otherwise
> > > parsing graph here is not really useful at all.
> >
> > I think using device_connection_find_match() for finding the graph becomes complicated.
> > The current arguments of the function is the below:
> >
> > void *device_connection_find_match(struct device *dev, const char *con_id,
> > 			       void *data,
> > 			       void *(*match)(struct device_connection *con,
> > 					      int ep, void *data))
> >
> > If finding the graph, the following arguments will be not used.
> >  - con_id
> >  - *con and ep of "match" arguments.
> >
> > This is because these arguments are for the build-in connections.
> 
> No they're not. You do realize that we can build a temporary struct
> device_connection there and then (in stack) to be supplied for the
> ->match callback.

I understood it.

> > So, should I modify the arguments of the function for finding both
> > the graph and built-in connections somehow?
> 
> I don't see any need for that. We may need to modify struct
> device_connection, but there should not be any need to touch the API.
> 
> Your plan to use port and endpoint number is wrong, as they are just
> indexes, and therefore not reliable. Luckily it should be completely
> unnecessary to use them.
> 
> The way I see this working is that we parse all the endpoints the
> caller device has. If we can't take advantage of the con_id for
> identification (though ideally we can), we just need to call ->match
> with every one of them. The implementation for the ->match callback is
> then responsible of figuring out if the endpoint is the one we are
> looking for or not in that case.

I understood it. But, I need to investigate how to find a device.

> The only problem I see with that is that we may not have a reliable
> way to convert the fwnode pointing to the remote-endpoint parent into
> struct device (if one has already been enumerated) in order to get the
> device name and use it as the second endpoint name in struct
> device_connection. To solve that, we could consider for example just
> adding a new member, fwnode pointer perhaps, to the structure. Or
> perhaps use the endpoint members for something else than device names
> when graph is used, and add a member defining the type of match:
> graph, build-in, etc. There are many things we can consider.

I don't understand why adding such new member(s) can solve that.
Anyway, I will investigate this more...

> I don't know if I'm able to explain what I'm after with this, so if
> you like, I can propose something for this myself. Though that will
> have to wait for few weeks. Right now I'm too busy with other stuff.

Thank you for your proposal! However, I'd like to try to investigate
once more while you are too busy.

Best regards,
Yoshihiro Shimoda

> 
> Thanks,
> 
> --
> heikki
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html