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@xxxxxxxxxxxxxx> > > --- > > 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
