| Message ID | 1557922152-16449-5-git-send-email-biju.das@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add USB3.0 and TI HD3SS3220 driver support | expand |
On Wed, 2019-05-15 at 13:09 +0100, Biju Das wrote: > The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220 usb > type-c drp port controller. This patch adds dual role switch support for > the type-c connector using the usb role switch class framework. > > Signed-off-by: Biju Das <biju.das@bp.renesas.com> > --- > V5-->V6 > * Added graph api's to find the role supported by the connector. > V4-->V5 > * Incorporated Shimoda-san's review comment > (https://patchwork.kernel.org/patch/10902537/) > V3-->V4 > * No Change > V2-->V3 > * Incorporated Shimoda-san's review comment > (https://patchwork.kernel.org/patch/10852507/) > * Used renesas,usb-role-switch property for differentiating USB > role switch associated with Type-C port controller driver. > V1-->V2 > * Driver uses usb role clas for handling dual role switch and handling > connect/disconnect events instead of extcon. > --- > drivers/usb/gadget/udc/renesas_usb3.c | 121 ++++++++++++++++++++++++++++++++-- > 1 file changed, 114 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > index 7dc2485..1d41998 100644 > --- a/drivers/usb/gadget/udc/renesas_usb3.c > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > @@ -24,6 +24,7 @@ > #include <linux/usb/ch9.h> > #include <linux/usb/gadget.h> > #include <linux/usb/of.h> > +#include <linux/of_graph.h> > #include <linux/usb/role.h> > > /* register definitions */ > @@ -351,6 +352,8 @@ struct renesas_usb3 { > int disabled_count; > > struct usb_request *ep0_req; > + > + enum usb_role connection_state; > u16 test_mode; > u8 ep0_buf[USB3_EP0_BUF_SIZE]; > bool softconnect; > @@ -359,6 +362,7 @@ struct renesas_usb3 { > bool extcon_usb; /* check vbus and set EXTCON_USB */ > bool forced_b_device; > bool start_to_connect; > + bool dual_role_sw; > }; > > #define gadget_to_renesas_usb3(_gadget) \ > @@ -699,8 +703,10 @@ static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool a_dev) > unsigned long flags; > > spin_lock_irqsave(&usb3->lock, flags); > - usb3_set_mode_by_role_sw(usb3, host); > - usb3_vbus_out(usb3, a_dev); > + if (!usb3->dual_role_sw || usb3->connection_state != USB_ROLE_NONE) { > + usb3_set_mode_by_role_sw(usb3, host); > + usb3_vbus_out(usb3, a_dev); > + } > /* for A-Peripheral or forced B-device mode */ > if ((!host && a_dev) || usb3->start_to_connect) > usb3_connect(usb3); > @@ -716,7 +722,8 @@ static void usb3_check_id(struct renesas_usb3 *usb3) > { > usb3->extcon_host = usb3_is_a_device(usb3); > > - if (usb3->extcon_host && !usb3->forced_b_device) > + if ((!usb3->dual_role_sw && usb3->extcon_host && !usb3->forced_b_device) > + || usb3->connection_state == USB_ROLE_HOST) > usb3_mode_config(usb3, true, true); > else > usb3_mode_config(usb3, false, false); > @@ -2343,14 +2350,65 @@ static enum usb_role renesas_usb3_role_switch_get(struct device *dev) > return cur_role; > } > > -static int renesas_usb3_role_switch_set(struct device *dev, > - enum usb_role role) > +static void handle_ext_role_switch_states(struct device *dev, > + enum usb_role role) > +{ > + struct renesas_usb3 *usb3 = dev_get_drvdata(dev); > + struct device *host = usb3->host_dev; > + enum usb_role cur_role = renesas_usb3_role_switch_get(dev); > + > + switch (role) { > + case USB_ROLE_NONE: > + usb3->connection_state = USB_ROLE_NONE; > + if (usb3->driver) > + usb3_disconnect(usb3); > + usb3_vbus_out(usb3, false); > + break; > + case USB_ROLE_DEVICE: > + if (usb3->connection_state == USB_ROLE_NONE) { > + usb3->connection_state = USB_ROLE_DEVICE; > + usb3_set_mode(usb3, false); > + if (usb3->driver) > + usb3_connect(usb3); > + } else if (cur_role == USB_ROLE_HOST) { > + device_release_driver(host); > + usb3_set_mode(usb3, false); > + if (usb3->driver) > + usb3_connect(usb3); > + } > + usb3_vbus_out(usb3, false); > + break; > + case USB_ROLE_HOST: > + if (usb3->connection_state == USB_ROLE_NONE) { > + if (usb3->driver) > + usb3_disconnect(usb3); > + > + usb3->connection_state = USB_ROLE_HOST; > + usb3_set_mode(usb3, true); > + usb3_vbus_out(usb3, true); > + if (device_attach(host) < 0) > + dev_err(dev, "device_attach(host) failed\n"); > + } else if (cur_role == USB_ROLE_DEVICE) { > + usb3_disconnect(usb3); > + /* Must set the mode before device_attach of the host */ > + usb3_set_mode(usb3, true); > + /* This device_attach() might sleep */ > + if (device_attach(host) < 0) > + dev_err(dev, "device_attach(host) failed\n"); > + } > + break; > + default: > + break; > + } > +} > + > +static void handle_role_switch_states(struct device *dev, > + enum usb_role role) > { > struct renesas_usb3 *usb3 = dev_get_drvdata(dev); > struct device *host = usb3->host_dev; > enum usb_role cur_role = renesas_usb3_role_switch_get(dev); > > - pm_runtime_get_sync(dev); > if (cur_role == USB_ROLE_HOST && role == USB_ROLE_DEVICE) { > device_release_driver(host); > usb3_set_mode(usb3, false); > @@ -2361,6 +2419,20 @@ static int renesas_usb3_role_switch_set(struct device *dev, > if (device_attach(host) < 0) > dev_err(dev, "device_attach(host) failed\n"); > } > +} > + > +static int renesas_usb3_role_switch_set(struct device *dev, > + enum usb_role role) > +{ > + struct renesas_usb3 *usb3 = dev_get_drvdata(dev); > + > + pm_runtime_get_sync(dev); > + > + if (usb3->dual_role_sw) > + handle_ext_role_switch_states(dev, role); > + else > + handle_role_switch_states(dev, role); > + > pm_runtime_put(dev); > > return 0; > @@ -2650,12 +2722,41 @@ static const unsigned int renesas_usb3_cable[] = { > EXTCON_NONE, > }; > > -static const struct usb_role_switch_desc renesas_usb3_role_switch_desc = { > +static struct usb_role_switch_desc renesas_usb3_role_switch_desc = { > .set = renesas_usb3_role_switch_set, > .get = renesas_usb3_role_switch_get, > .allow_userspace_control = true, > }; > > +static bool is_usb_dual_role_switch(struct device *dev) To me, it's not good idea to pay an attention to specific consumer of the role switch, assume any device could assign role if it get this USB role switch, not only type-c connector > +{ > + struct device_node *np = dev->of_node; > + struct device_node *parent; > + struct device_node *child; > + bool ret = false; > + const char *role_type = NULL; > + > + child = of_graph_get_endpoint_by_regs(np, -1, -1); > + if (!child) > + return ret; > + > + parent = of_graph_get_remote_port_parent(child); > + of_node_put(child); > + child = of_get_child_by_name(parent, "connector"); > + of_node_put(parent); > + if (!child) > + return ret; > + > + if (of_device_is_compatible(child, "usb-c-connector")) { > + of_property_read_string(child, "data-role", &role_type); > + if (role_type && (!strncmp(role_type, "dual", strlen("dual")))) > + ret = true; > + } > + > + of_node_put(child); > + return ret; > +} > + > static int renesas_usb3_probe(struct platform_device *pdev) > { > struct renesas_usb3 *usb3; > @@ -2741,6 +2842,12 @@ static int renesas_usb3_probe(struct platform_device *pdev) > if (ret < 0) > goto err_dev_create; > > + if (device_property_read_bool(&pdev->dev, "usb-role-switch") && > + is_usb_dual_role_switch(&pdev->dev)) { > + usb3->dual_role_sw = true; > + renesas_usb3_role_switch_desc.fwnode = dev_fwnode(&pdev->dev); > + } > + > INIT_WORK(&usb3->role_work, renesas_usb3_role_work); > usb3->role_sw = usb_role_switch_register(&pdev->dev, > &renesas_usb3_role_switch_desc);
+ Rob Hi Chunfeng Yun, Thanks for the feedback. > Subject: Re: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role > switch support > > On Wed, 2019-05-15 at 13:09 +0100, Biju Das wrote: > > The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220 > > usb type-c drp port controller. This patch adds dual role switch > > support for the type-c connector using the usb role switch class framework. > > > > Signed-off-by: Biju Das <biju.das@bp.renesas.com> > > --- > > V5-->V6 > > * Added graph api's to find the role supported by the connector. > > V4-->V5 > > * Incorporated Shimoda-san's review comment > > (https://patchwork.kernel.org/patch/10902537/) > > V3-->V4 > > * No Change > > V2-->V3 > > * Incorporated Shimoda-san's review comment > > (https://patchwork.kernel.org/patch/10852507/) > > * Used renesas,usb-role-switch property for differentiating USB > > role switch associated with Type-C port controller driver. > > V1-->V2 > > * Driver uses usb role clas for handling dual role switch and handling > > connect/disconnect events instead of extcon. > > --- > > drivers/usb/gadget/udc/renesas_usb3.c | 121 > > ++++++++++++++++++++++++++++++++-- > > 1 file changed, 114 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c > > b/drivers/usb/gadget/udc/renesas_usb3.c > > index 7dc2485..1d41998 100644 > > --- a/drivers/usb/gadget/udc/renesas_usb3.c > > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > > @@ -24,6 +24,7 @@ > > #include <linux/usb/ch9.h> > > #include <linux/usb/gadget.h> > > #include <linux/usb/of.h> > > +#include <linux/of_graph.h> > > #include <linux/usb/role.h> > > > > /* register definitions */ > > @@ -351,6 +352,8 @@ struct renesas_usb3 { > > int disabled_count; > > > > struct usb_request *ep0_req; > > + > > + enum usb_role connection_state; > > u16 test_mode; > > u8 ep0_buf[USB3_EP0_BUF_SIZE]; > > bool softconnect; > > @@ -359,6 +362,7 @@ struct renesas_usb3 { > > bool extcon_usb; /* check vbus and set EXTCON_USB > */ > > bool forced_b_device; > > bool start_to_connect; > > + bool dual_role_sw; > > }; > > > > #define gadget_to_renesas_usb3(_gadget) \ > > @@ -699,8 +703,10 @@ static void usb3_mode_config(struct renesas_usb3 > *usb3, bool host, bool a_dev) > > unsigned long flags; > > > > spin_lock_irqsave(&usb3->lock, flags); > > - usb3_set_mode_by_role_sw(usb3, host); > > - usb3_vbus_out(usb3, a_dev); > > + if (!usb3->dual_role_sw || usb3->connection_state != > USB_ROLE_NONE) { > > + usb3_set_mode_by_role_sw(usb3, host); > > + usb3_vbus_out(usb3, a_dev); > > + } > > /* for A-Peripheral or forced B-device mode */ > > if ((!host && a_dev) || usb3->start_to_connect) > > usb3_connect(usb3); > > @@ -716,7 +722,8 @@ static void usb3_check_id(struct renesas_usb3 > > *usb3) { > > usb3->extcon_host = usb3_is_a_device(usb3); > > > > - if (usb3->extcon_host && !usb3->forced_b_device) > > + if ((!usb3->dual_role_sw && usb3->extcon_host && !usb3- > >forced_b_device) > > + || usb3->connection_state == USB_ROLE_HOST) > > usb3_mode_config(usb3, true, true); > > else > > usb3_mode_config(usb3, false, false); @@ -2343,14 +2350,65 > @@ > > static enum usb_role renesas_usb3_role_switch_get(struct device *dev) > > return cur_role; > > } > > > > -static int renesas_usb3_role_switch_set(struct device *dev, > > - enum usb_role role) > > +static void handle_ext_role_switch_states(struct device *dev, > > + enum usb_role role) > > +{ > > + struct renesas_usb3 *usb3 = dev_get_drvdata(dev); > > + struct device *host = usb3->host_dev; > > + enum usb_role cur_role = renesas_usb3_role_switch_get(dev); > > + > > + switch (role) { > > + case USB_ROLE_NONE: > > + usb3->connection_state = USB_ROLE_NONE; > > + if (usb3->driver) > > + usb3_disconnect(usb3); > > + usb3_vbus_out(usb3, false); > > + break; > > + case USB_ROLE_DEVICE: > > + if (usb3->connection_state == USB_ROLE_NONE) { > > + usb3->connection_state = USB_ROLE_DEVICE; > > + usb3_set_mode(usb3, false); > > + if (usb3->driver) > > + usb3_connect(usb3); > > + } else if (cur_role == USB_ROLE_HOST) { > > + device_release_driver(host); > > + usb3_set_mode(usb3, false); > > + if (usb3->driver) > > + usb3_connect(usb3); > > + } > > + usb3_vbus_out(usb3, false); > > + break; > > + case USB_ROLE_HOST: > > + if (usb3->connection_state == USB_ROLE_NONE) { > > + if (usb3->driver) > > + usb3_disconnect(usb3); > > + > > + usb3->connection_state = USB_ROLE_HOST; > > + usb3_set_mode(usb3, true); > > + usb3_vbus_out(usb3, true); > > + if (device_attach(host) < 0) > > + dev_err(dev, "device_attach(host) > failed\n"); > > + } else if (cur_role == USB_ROLE_DEVICE) { > > + usb3_disconnect(usb3); > > + /* Must set the mode before device_attach of the > host */ > > + usb3_set_mode(usb3, true); > > + /* This device_attach() might sleep */ > > + if (device_attach(host) < 0) > > + dev_err(dev, "device_attach(host) > failed\n"); > > + } > > + break; > > + default: > > + break; > > + } > > +} > > + > > +static void handle_role_switch_states(struct device *dev, > > + enum usb_role role) > > { > > struct renesas_usb3 *usb3 = dev_get_drvdata(dev); > > struct device *host = usb3->host_dev; > > enum usb_role cur_role = renesas_usb3_role_switch_get(dev); > > > > - pm_runtime_get_sync(dev); > > if (cur_role == USB_ROLE_HOST && role == USB_ROLE_DEVICE) { > > device_release_driver(host); > > usb3_set_mode(usb3, false); > > @@ -2361,6 +2419,20 @@ static int renesas_usb3_role_switch_set(struct > device *dev, > > if (device_attach(host) < 0) > > dev_err(dev, "device_attach(host) failed\n"); > > } > > +} > > + > > +static int renesas_usb3_role_switch_set(struct device *dev, > > + enum usb_role role) > > +{ > > + struct renesas_usb3 *usb3 = dev_get_drvdata(dev); > > + > > + pm_runtime_get_sync(dev); > > + > > + if (usb3->dual_role_sw) > > + handle_ext_role_switch_states(dev, role); > > + else > > + handle_role_switch_states(dev, role); > > + > > pm_runtime_put(dev); > > > > return 0; > > @@ -2650,12 +2722,41 @@ static const unsigned int renesas_usb3_cable[] > = { > > EXTCON_NONE, > > }; > > > > -static const struct usb_role_switch_desc > > renesas_usb3_role_switch_desc = { > > +static struct usb_role_switch_desc renesas_usb3_role_switch_desc = { > > .set = renesas_usb3_role_switch_set, > > .get = renesas_usb3_role_switch_get, > > .allow_userspace_control = true, > > }; > > > > +static bool is_usb_dual_role_switch(struct device *dev) > To me, it's not good idea to pay an attention to specific consumer of the role > switch, assume any device could assign role if it get this USB role switch, not > only type-c connector Yes, I agree. I have previously posted a patch based on this [1]. [1]. https://patchwork.kernel.org/patch/10852505/ Then on the binding patch, Rob suggested to walk the graph to the connector and determine if dual role is supported by the connector type [2] [2]. https://patchwork.kernel.org/patch/10914379/ The purpose of "usb role switch" is to assign the roles. So looks like, we don't need this function. Please correct me, if I am wrong. Regards, Biju > > +{ > > + struct device_node *np = dev->of_node; > > + struct device_node *parent; > > + struct device_node *child; > > + bool ret = false; > > + const char *role_type = NULL; > > + > > + child = of_graph_get_endpoint_by_regs(np, -1, -1); > > + if (!child) > > + return ret; > > + > > + parent = of_graph_get_remote_port_parent(child); > > + of_node_put(child); > > + child = of_get_child_by_name(parent, "connector"); > > + of_node_put(parent); > > + if (!child) > > + return ret; > > + > > + if (of_device_is_compatible(child, "usb-c-connector")) { > > + of_property_read_string(child, "data-role", &role_type); > > + if (role_type && (!strncmp(role_type, "dual", > strlen("dual")))) > > + ret = true; > > + } > > + > > + of_node_put(child); > > + return ret; > > +} > > + > > static int renesas_usb3_probe(struct platform_device *pdev) { > > struct renesas_usb3 *usb3; > > @@ -2741,6 +2842,12 @@ static int renesas_usb3_probe(struct > platform_device *pdev) > > if (ret < 0) > > goto err_dev_create; > > > > + if (device_property_read_bool(&pdev->dev, "usb-role-switch") && > > + is_usb_dual_role_switch(&pdev->dev)) { > > + usb3->dual_role_sw = true; > > + renesas_usb3_role_switch_desc.fwnode = > dev_fwnode(&pdev->dev); > > + } > > + > > INIT_WORK(&usb3->role_work, renesas_usb3_role_work); > > usb3->role_sw = usb_role_switch_register(&pdev->dev, > > &renesas_usb3_role_switch_desc); >
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
Hi Shimoda-San, Thanks for the feedback. > > From: Biju Das, Sent: Wednesday, May 15, 2019 9:09 PM > > Subject: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role > > switch support > > Now I'm confusing about the "Add dual role switch support" mean... > Especially, this driver has already supports dual role switch support by own > sysfs or debugfs. Sorry for the confusion. What about "Enhance role switch support" ? > > The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220 > > usb type-c drp port controller. This patch adds dual role switch > > support for the type-c connector using the usb role switch class framework. > > IIUC, after this patch is applied, the hs3ss3220 type-c driver can switch the > role by using the usb role switch class framework. Yes, That is correct. HD3SS3220 driver detects host/device connection events (attach/detach) and It calls "usb_role_switch_set_role" to assign/switch the role. > > Signed-off-by: Biju Das <biju.das@bp.renesas.com> > > --- > > V5-->V6 > > * Added graph api's to find the role supported by the connector. > > V4-->V5 > > * Incorporated Shimoda-san's review comment > > (https://patchwork.kernel.org/patch/10902537/) > > V3-->V4 > > * No Change > > V2-->V3 > > * Incorporated Shimoda-san's review comment > > (https://patchwork.kernel.org/patch/10852507/) > > * Used renesas,usb-role-switch property for differentiating USB > > role switch associated with Type-C port controller driver. > > V1-->V2 > > * Driver uses usb role clas for handling dual role switch and handling > > connect/disconnect events instead of extcon. > > --- > > drivers/usb/gadget/udc/renesas_usb3.c | 121 > > ++++++++++++++++++++++++++++++++-- > > 1 file changed, 114 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c > > b/drivers/usb/gadget/udc/renesas_usb3.c > > index 7dc2485..1d41998 100644 > > --- a/drivers/usb/gadget/udc/renesas_usb3.c > > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > > @@ -24,6 +24,7 @@ > > #include <linux/usb/ch9.h> > > #include <linux/usb/gadget.h> > > #include <linux/usb/of.h> > > +#include <linux/of_graph.h> > > #include <linux/usb/role.h> > > > > /* register definitions */ > > @@ -351,6 +352,8 @@ struct renesas_usb3 { > > int disabled_count; > > > > struct usb_request *ep0_req; > > + > > + enum usb_role connection_state; > > u16 test_mode; > > u8 ep0_buf[USB3_EP0_BUF_SIZE]; > > bool softconnect; > > @@ -359,6 +362,7 @@ struct renesas_usb3 { > > bool extcon_usb; /* check vbus and set EXTCON_USB > */ > > bool forced_b_device; > > bool start_to_connect; > > + bool dual_role_sw; > > I'm also confusing this name. OK. will change it to "role_sw_by_connector" > > }; > > > > #define gadget_to_renesas_usb3(_gadget) \ > <snip> > > @@ -699,8 +703,10 @@ static void usb3_mode_config(struct renesas_usb3 > *usb3, bool host, bool a_dev) > > unsigned long flags; > > > > spin_lock_irqsave(&usb3->lock, flags); > > - usb3_set_mode_by_role_sw(usb3, host); > > - usb3_vbus_out(usb3, a_dev); > > + if (!usb3->dual_role_sw || usb3->connection_state != > USB_ROLE_NONE) { > > + usb3_set_mode_by_role_sw(usb3, host); > > + usb3_vbus_out(usb3, a_dev); > > + } > > /* for A-Peripheral or forced B-device mode */ > > if ((!host && a_dev) || usb3->start_to_connect) > > usb3_connect(usb3); > > @@ -716,7 +722,8 @@ static void usb3_check_id(struct renesas_usb3 > > *usb3) { > > usb3->extcon_host = usb3_is_a_device(usb3); > > > > - if (usb3->extcon_host && !usb3->forced_b_device) > > + if ((!usb3->dual_role_sw && usb3->extcon_host && !usb3- > >forced_b_device) > > + || usb3->connection_state == USB_ROLE_HOST) > > usb3_mode_config(usb3, true, true); > > else > > usb3_mode_config(usb3, false, false); @@ -2343,14 +2350,65 > @@ > > static enum usb_role renesas_usb3_role_switch_get(struct device *dev) > > return cur_role; > > } > > > > -static int renesas_usb3_role_switch_set(struct device *dev, > > - enum usb_role role) > > +static void handle_ext_role_switch_states(struct device *dev, > > + enum usb_role role) > > +{ > > + struct renesas_usb3 *usb3 = dev_get_drvdata(dev); > > + struct device *host = usb3->host_dev; > > + enum usb_role cur_role = renesas_usb3_role_switch_get(dev); > > + > > + switch (role) { > > + case USB_ROLE_NONE: > > + usb3->connection_state = USB_ROLE_NONE; > > + if (usb3->driver) > > + usb3_disconnect(usb3); > > + usb3_vbus_out(usb3, false); > > + break; > > + case USB_ROLE_DEVICE: > > + if (usb3->connection_state == USB_ROLE_NONE) { > > + usb3->connection_state = USB_ROLE_DEVICE; > > + usb3_set_mode(usb3, false); > > + if (usb3->driver) > > + usb3_connect(usb3); > > + } else if (cur_role == USB_ROLE_HOST) { > > + device_release_driver(host); > > + usb3_set_mode(usb3, false); > > + if (usb3->driver) > > + usb3_connect(usb3); > > + } > > + usb3_vbus_out(usb3, false); > > + break; > > + case USB_ROLE_HOST: > > + if (usb3->connection_state == USB_ROLE_NONE) { > > + if (usb3->driver) > > + usb3_disconnect(usb3); > > + > > + usb3->connection_state = USB_ROLE_HOST; > > + usb3_set_mode(usb3, true); > > + usb3_vbus_out(usb3, true); > > + if (device_attach(host) < 0) > > + dev_err(dev, "device_attach(host) > failed\n"); > > + } else if (cur_role == USB_ROLE_DEVICE) { > > + usb3_disconnect(usb3); > > + /* Must set the mode before device_attach of the > host */ > > + usb3_set_mode(usb3, true); > > + /* This device_attach() might sleep */ > > + if (device_attach(host) < 0) > > + dev_err(dev, "device_attach(host) > failed\n"); > > + } > > + break; > > + default: > > + break; > > + } > > +} > > + > > +static void handle_role_switch_states(struct device *dev, > > + enum usb_role role) > > { > > struct renesas_usb3 *usb3 = dev_get_drvdata(dev); > > struct device *host = usb3->host_dev; > > enum usb_role cur_role = renesas_usb3_role_switch_get(dev); > > > > - pm_runtime_get_sync(dev); > > if (cur_role == USB_ROLE_HOST && role == USB_ROLE_DEVICE) { > > device_release_driver(host); > > usb3_set_mode(usb3, false); > > @@ -2361,6 +2419,20 @@ static int renesas_usb3_role_switch_set(struct > device *dev, > > if (device_attach(host) < 0) > > dev_err(dev, "device_attach(host) failed\n"); > > } > > +} > > + > > +static int renesas_usb3_role_switch_set(struct device *dev, > > + enum usb_role role) > > +{ > > + struct renesas_usb3 *usb3 = dev_get_drvdata(dev); > > + > > + pm_runtime_get_sync(dev); > > + > > + if (usb3->dual_role_sw) > > + handle_ext_role_switch_states(dev, role); > > + else > > + handle_role_switch_states(dev, role); > > + > > pm_runtime_put(dev); > > > > return 0; > > @@ -2650,12 +2722,41 @@ static const unsigned int renesas_usb3_cable[] > = { > > EXTCON_NONE, > > }; > > > > -static const struct usb_role_switch_desc > > renesas_usb3_role_switch_desc = { > > +static struct usb_role_switch_desc renesas_usb3_role_switch_desc = { > > .set = renesas_usb3_role_switch_set, > > .get = renesas_usb3_role_switch_get, > > .allow_userspace_control = true, > > }; > > > > +static bool is_usb_dual_role_switch(struct device *dev) { > > + struct device_node *np = dev->of_node; > > + struct device_node *parent; > > + struct device_node *child; > > + bool ret = false; > > + const char *role_type = NULL; > > + > > + child = of_graph_get_endpoint_by_regs(np, -1, -1); > > + if (!child) > > + return ret; > > + > > + parent = of_graph_get_remote_port_parent(child); > > + of_node_put(child); > > + child = of_get_child_by_name(parent, "connector"); > > + of_node_put(parent); > > + if (!child) > > + return ret; > > + > > + if (of_device_is_compatible(child, "usb-c-connector")) { > > + of_property_read_string(child, "data-role", &role_type); > > + if (role_type && (!strncmp(role_type, "dual", > strlen("dual")))) > > + ret = true; > > + } > > + > > + of_node_put(child); > > + return ret; > > +} > > + > > static int renesas_usb3_probe(struct platform_device *pdev) { > > struct renesas_usb3 *usb3; > > @@ -2741,6 +2842,12 @@ static int renesas_usb3_probe(struct > platform_device *pdev) > > if (ret < 0) > > goto err_dev_create; > > > > + if (device_property_read_bool(&pdev->dev, "usb-role-switch") && > > + is_usb_dual_role_switch(&pdev->dev)) { > > I think either one of the conditions is enough. (Only "usb-role-switch" > checking is enough, IIUC). OK, Will remove the other check. > JFYI, according to the binding document [1], this "usb-role-switch" means: > --- > + - usb-role-switch: boolean, indicates that the device is capable of assigning > + the USB data role (USB host or USB device) for a > given > + USB connector, such as Type-C, Type-B(micro). > + see connector/usb-connector.txt. > --- > > So, R-Car Gen3 / Salvator-XS cannot have this property because the board > has Type-A connector. > > [1] https://patchwork.kernel.org/patch/10934835/ I have updated the binding patch to cover this[1] [1]. https://patchwork.kernel.org/patch/10944631/ Are you ok with this binding patch? > > + usb3->dual_role_sw = true; > > So, "role_sw_by_connector" matches with my image. > What do you think? OK, will use " role_sw_by_connector" Regards, Biju > > + renesas_usb3_role_switch_desc.fwnode = > dev_fwnode(&pdev->dev); > > + } > > + > > INIT_WORK(&usb3->role_work, renesas_usb3_role_work); > > usb3->role_sw = usb_role_switch_register(&pdev->dev, > > &renesas_usb3_role_switch_desc); > > -- > > 2.7.4
Hi Biju-san, > From: Biju Das, Sent: Tuesday, May 21, 2019 4:10 PM > > Hi Shimoda-San, > > Thanks for the feedback. > > > > From: Biju Das, Sent: Wednesday, May 15, 2019 9:09 PM > > > Subject: [PATCH v6 4/7] usb: gadget: udc: renesas_usb3: Add dual role > > > switch support > > > > Now I'm confusing about the "Add dual role switch support" mean... > > Especially, this driver has already supports dual role switch support by own > > sysfs or debugfs. > > Sorry for the confusion. > What about "Enhance role switch support" ? Thank you for the suggestion. It's good to me. > > > The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220 > > > usb type-c drp port controller. This patch adds dual role switch > > > support for the type-c connector using the usb role switch class framework. > > > > IIUC, after this patch is applied, the hs3ss3220 type-c driver can switch the > > role by using the usb role switch class framework. > > Yes, That is correct. HD3SS3220 driver detects host/device connection events (attach/detach) and > It calls "usb_role_switch_set_role" to assign/switch the role. I got it. <snip> > > JFYI, according to the binding document [1], this "usb-role-switch" means: > > --- > > + - usb-role-switch: boolean, indicates that the device is capable of assigning > > + the USB data role (USB host or USB device) for a > > given > > + USB connector, such as Type-C, Type-B(micro). > > + see connector/usb-connector.txt. > > --- > > > > So, R-Car Gen3 / Salvator-XS cannot have this property because the board > > has Type-A connector. > > > > [1] https://patchwork.kernel.org/patch/10934835/ > > I have updated the binding patch to cover this[1] > [1]. https://patchwork.kernel.org/patch/10944631/ > Are you ok with this binding patch? I'm sorry, I overlooked the patch. I'll check it later. Best regards, Yoshihiro Shimoda
diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c index 7dc2485..1d41998 100644 --- a/drivers/usb/gadget/udc/renesas_usb3.c +++ b/drivers/usb/gadget/udc/renesas_usb3.c @@ -24,6 +24,7 @@ #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb/of.h> +#include <linux/of_graph.h> #include <linux/usb/role.h> /* register definitions */ @@ -351,6 +352,8 @@ struct renesas_usb3 { int disabled_count; struct usb_request *ep0_req; + + enum usb_role connection_state; u16 test_mode; u8 ep0_buf[USB3_EP0_BUF_SIZE]; bool softconnect; @@ -359,6 +362,7 @@ struct renesas_usb3 { bool extcon_usb; /* check vbus and set EXTCON_USB */ bool forced_b_device; bool start_to_connect; + bool dual_role_sw; }; #define gadget_to_renesas_usb3(_gadget) \ @@ -699,8 +703,10 @@ static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool a_dev) unsigned long flags; spin_lock_irqsave(&usb3->lock, flags); - usb3_set_mode_by_role_sw(usb3, host); - usb3_vbus_out(usb3, a_dev); + if (!usb3->dual_role_sw || usb3->connection_state != USB_ROLE_NONE) { + usb3_set_mode_by_role_sw(usb3, host); + usb3_vbus_out(usb3, a_dev); + } /* for A-Peripheral or forced B-device mode */ if ((!host && a_dev) || usb3->start_to_connect) usb3_connect(usb3); @@ -716,7 +722,8 @@ static void usb3_check_id(struct renesas_usb3 *usb3) { usb3->extcon_host = usb3_is_a_device(usb3); - if (usb3->extcon_host && !usb3->forced_b_device) + if ((!usb3->dual_role_sw && usb3->extcon_host && !usb3->forced_b_device) + || usb3->connection_state == USB_ROLE_HOST) usb3_mode_config(usb3, true, true); else usb3_mode_config(usb3, false, false); @@ -2343,14 +2350,65 @@ static enum usb_role renesas_usb3_role_switch_get(struct device *dev) return cur_role; } -static int renesas_usb3_role_switch_set(struct device *dev, - enum usb_role role) +static void handle_ext_role_switch_states(struct device *dev, + enum usb_role role) +{ + struct renesas_usb3 *usb3 = dev_get_drvdata(dev); + struct device *host = usb3->host_dev; + enum usb_role cur_role = renesas_usb3_role_switch_get(dev); + + switch (role) { + case USB_ROLE_NONE: + usb3->connection_state = USB_ROLE_NONE; + if (usb3->driver) + usb3_disconnect(usb3); + usb3_vbus_out(usb3, false); + break; + case USB_ROLE_DEVICE: + if (usb3->connection_state == USB_ROLE_NONE) { + usb3->connection_state = USB_ROLE_DEVICE; + usb3_set_mode(usb3, false); + if (usb3->driver) + usb3_connect(usb3); + } else if (cur_role == USB_ROLE_HOST) { + device_release_driver(host); + usb3_set_mode(usb3, false); + if (usb3->driver) + usb3_connect(usb3); + } + usb3_vbus_out(usb3, false); + break; + case USB_ROLE_HOST: + if (usb3->connection_state == USB_ROLE_NONE) { + if (usb3->driver) + usb3_disconnect(usb3); + + usb3->connection_state = USB_ROLE_HOST; + usb3_set_mode(usb3, true); + usb3_vbus_out(usb3, true); + if (device_attach(host) < 0) + dev_err(dev, "device_attach(host) failed\n"); + } else if (cur_role == USB_ROLE_DEVICE) { + usb3_disconnect(usb3); + /* Must set the mode before device_attach of the host */ + usb3_set_mode(usb3, true); + /* This device_attach() might sleep */ + if (device_attach(host) < 0) + dev_err(dev, "device_attach(host) failed\n"); + } + break; + default: + break; + } +} + +static void handle_role_switch_states(struct device *dev, + enum usb_role role) { struct renesas_usb3 *usb3 = dev_get_drvdata(dev); struct device *host = usb3->host_dev; enum usb_role cur_role = renesas_usb3_role_switch_get(dev); - pm_runtime_get_sync(dev); if (cur_role == USB_ROLE_HOST && role == USB_ROLE_DEVICE) { device_release_driver(host); usb3_set_mode(usb3, false); @@ -2361,6 +2419,20 @@ static int renesas_usb3_role_switch_set(struct device *dev, if (device_attach(host) < 0) dev_err(dev, "device_attach(host) failed\n"); } +} + +static int renesas_usb3_role_switch_set(struct device *dev, + enum usb_role role) +{ + struct renesas_usb3 *usb3 = dev_get_drvdata(dev); + + pm_runtime_get_sync(dev); + + if (usb3->dual_role_sw) + handle_ext_role_switch_states(dev, role); + else + handle_role_switch_states(dev, role); + pm_runtime_put(dev); return 0; @@ -2650,12 +2722,41 @@ static const unsigned int renesas_usb3_cable[] = { EXTCON_NONE, }; -static const struct usb_role_switch_desc renesas_usb3_role_switch_desc = { +static struct usb_role_switch_desc renesas_usb3_role_switch_desc = { .set = renesas_usb3_role_switch_set, .get = renesas_usb3_role_switch_get, .allow_userspace_control = true, }; +static bool is_usb_dual_role_switch(struct device *dev) +{ + struct device_node *np = dev->of_node; + struct device_node *parent; + struct device_node *child; + bool ret = false; + const char *role_type = NULL; + + child = of_graph_get_endpoint_by_regs(np, -1, -1); + if (!child) + return ret; + + parent = of_graph_get_remote_port_parent(child); + of_node_put(child); + child = of_get_child_by_name(parent, "connector"); + of_node_put(parent); + if (!child) + return ret; + + if (of_device_is_compatible(child, "usb-c-connector")) { + of_property_read_string(child, "data-role", &role_type); + if (role_type && (!strncmp(role_type, "dual", strlen("dual")))) + ret = true; + } + + of_node_put(child); + return ret; +} + static int renesas_usb3_probe(struct platform_device *pdev) { struct renesas_usb3 *usb3; @@ -2741,6 +2842,12 @@ static int renesas_usb3_probe(struct platform_device *pdev) if (ret < 0) goto err_dev_create; + if (device_property_read_bool(&pdev->dev, "usb-role-switch") && + is_usb_dual_role_switch(&pdev->dev)) { + usb3->dual_role_sw = true; + renesas_usb3_role_switch_desc.fwnode = dev_fwnode(&pdev->dev); + } + INIT_WORK(&usb3->role_work, renesas_usb3_role_work); usb3->role_sw = usb_role_switch_register(&pdev->dev, &renesas_usb3_role_switch_desc);
The RZ/G2E cat874 board has a type-c connector connected to hd3ss3220 usb type-c drp port controller. This patch adds dual role switch support for the type-c connector using the usb role switch class framework. Signed-off-by: Biju Das <biju.das@bp.renesas.com> --- V5-->V6 * Added graph api's to find the role supported by the connector. V4-->V5 * Incorporated Shimoda-san's review comment (https://patchwork.kernel.org/patch/10902537/) V3-->V4 * No Change V2-->V3 * Incorporated Shimoda-san's review comment (https://patchwork.kernel.org/patch/10852507/) * Used renesas,usb-role-switch property for differentiating USB role switch associated with Type-C port controller driver. V1-->V2 * Driver uses usb role clas for handling dual role switch and handling connect/disconnect events instead of extcon. --- drivers/usb/gadget/udc/renesas_usb3.c | 121 ++++++++++++++++++++++++++++++++-- 1 file changed, 114 insertions(+), 7 deletions(-)