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

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

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