Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
To: Biju Das <biju.das@bp.renesas.com>,
	Felipe Balbi <balbi@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Biju Das <biju.das@bp.renesas.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Simon Horman <horms+renesas@verge.net.au>,
	Fabrizio Castro <fabrizio.castro@bp.renesas.com>,
	Kees Cook <keescook@chromium.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	Simon Horman <horms@verge.net.au>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Chris Paterson <Chris.Paterson2@renesas.com>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role switch support
Date: Tue, 21 May 2019 05:49:01 +0000
Message-ID: <OSAPR01MB30890B5983EEAD2536D01A05D8070@OSAPR01MB3089.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <1557922152-16449-5-git-send-email-biju.das@bp.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


  parent reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-15 12:09 [PATCH v6 0/7] Add USB3.0 and TI HD3SS3220 driver support Biju Das
2019-05-15 12:09 ` [PATCH v6 1/7] dt-bindings: usb: hd3ss3220 device tree binding document Biju Das
2019-05-15 16:09   ` Sergei Shtylyov
2019-05-21  9:42     ` Biju Das
2019-05-21  8:03   ` Yoshihiro Shimoda
2019-05-21  8:08     ` Kuninori Morimoto
2019-05-22 10:59       ` Biju Das
2019-05-23  5:18         ` Yoshihiro Shimoda
2019-05-23  8:56           ` Biju Das
2019-05-15 12:09 ` [PATCH v6 2/7] dt-bindings: usb: renesas_usb3: Document usb role switch support Biju Das
2019-05-21  8:27   ` Yoshihiro Shimoda
2019-05-28  7:25     ` Biju Das
2019-05-15 12:09 ` [PATCH v6 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller Biju Das
2019-05-15 12:09 ` [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role switch support Biju Das
2019-05-16  3:16   ` Chunfeng Yun
2019-05-16  8:00     ` Biju Das
2019-05-21  5:49   ` Yoshihiro Shimoda [this message]
2019-05-21  7:10     ` Biju Das
2019-05-21  7:34       ` Yoshihiro Shimoda
2019-05-15 12:09 ` [PATCH v6 5/7] arm64: defconfig: enable TYPEC_HD3SS3220 config option Biju Das
2019-05-15 12:09 ` [PATCH v6 6/7] arm64: dts: renesas: r8a774c0-cat874: Enable USB3.0 host/peripheral device node Biju Das
2019-05-15 12:09 ` [PATCH v6 7/7] arm64: dts: renesas: r8a774c0-cat874: Enable usb role switch support Biju Das
2019-05-24 21:50   ` Rob Herring

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=OSAPR01MB30890B5983EEAD2536D01A05D8070@OSAPR01MB3089.jpnprd01.prod.outlook.com \
    --to=yoshihiro.shimoda.uh@renesas.com \
    --cc=Chris.Paterson2@renesas.com \
    --cc=balbi@kernel.org \
    --cc=biju.das@bp.renesas.com \
    --cc=fabrizio.castro@bp.renesas.com \
    --cc=geert+renesas@glider.be \
    --cc=gregkh@linuxfoundation.org \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=horms+renesas@verge.net.au \
    --cc=horms@verge.net.au \
    --cc=keescook@chromium.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox