| Message ID | 1555407487-35394-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 |
Hi Biju-san, Thank you for the patch! > From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM > > RZ/G2E cat874 board is capable of detecting cable connect and disconnect > events. Add support for renesas_usb3 to receive connect and disconnect > events and support dual-role switch using usb-role-switch framework. > > Signed-off-by: Biju Das <biju.das@bp.renesas.com> > --- > 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. <snip> > +static void usb3_check_vbus(struct renesas_usb3 *usb3) > +{ > + if (usb3->workaround_for_vbus) { > + if (usb3->usb_role_switch_property) { > + if (usb3->connection_state == USB_ROLE_DEVICE) { > + usb3_mode_config(usb3, false, false); I should have pointed it out the previous version though, why does this usb3_mode_config() calling need? My guess is: - renesas_usb3_start() calls renesas_usb3_init_controller(). -- renesas_usb3_init_controller() calls usb3_check_id() and then usb_check_vbus(). --- usb_check_id() calls usb3_mode_config(usb3, true, true) and then the HW acts as host mode. ----> So, you'd like the HW to acts as peripheral mode when the connection_state is USB_ROLE_DEVICE, you added that the usb3_check_vbus() calls usb3_mode_config(usb3, false, false). Is my guess correct? If so, I'd like to add such code into usb3_check_id() like below: if ((usb3->extcon_host && !usb3->forced_b_device) || (usb3->usb_role_switch_property && usb3->connection_state == USB_ROLE_HOST)) usb3_mode_config(usb3, true, true); else usb3_mode_config(usb3, false, false); What do you think? Best regards, Yoshihiro Shimoda
Hi Shimoda-San, Thanks for the feedback. > Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use > usb_role_switch framework > > Hi Biju-san, > > Thank you for the patch! > > > From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM > > > > RZ/G2E cat874 board is capable of detecting cable connect and > > disconnect events. Add support for renesas_usb3 to receive connect and > > disconnect events and support dual-role switch using usb-role-switch > framework. > > > > Signed-off-by: Biju Das <biju.das@bp.renesas.com> > > --- > > 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. > <snip> > > > +static void usb3_check_vbus(struct renesas_usb3 *usb3) { > > + if (usb3->workaround_for_vbus) { > > + if (usb3->usb_role_switch_property) { > > + if (usb3->connection_state == USB_ROLE_DEVICE) { > > + usb3_mode_config(usb3, false, false); > > I should have pointed it out the previous version though, why does this > usb3_mode_config() calling need? > My guess is: > - renesas_usb3_start() calls renesas_usb3_init_controller(). > -- renesas_usb3_init_controller() calls usb3_check_id() and then > usb_check_vbus(). > --- usb_check_id() calls usb3_mode_config(usb3, true, true) and then the > HW acts as host mode. > ----> So, you'd like the HW to acts as peripheral mode when the > connection_state is USB_ROLE_DEVICE, > you added that the usb3_check_vbus() calls usb3_mode_config(usb3, > false, false). > > Is my guess correct? If so, I'd like to add such code into usb3_check_id() like > below: Yes, it is almost correct. The scenario I am trying is [1] USB type C cable connected to a Host Machine(TI chip identifies as Device connection. But we haven't installed Gadget module for Device operation) [2] After that trying to install gadget module. In this case, it calls usb_check_id() as mentioned above and configure it as Host mode. > if ((usb3->extcon_host && !usb3->forced_b_device) || > (usb3->usb_role_switch_property && > usb3->connection_state == USB_ROLE_HOST)) > usb3_mode_config(usb3, true, true); > else > usb3_mode_config(usb3, false, false); > > What do you think? Since as per [1], usb3->extcon_host=1 and !usb3->forced_b_device ==1 , The above code always enter into Host Mode configuration. To make it work, I need to update " usb3->forced_b_device" based on connection_state from TI chip. So the new code look like 1) There is no change in usb_check_id() call. if (usb3->extcon_host && !usb3->forced_b_device) usb3_mode_config(usb3, true, true); else usb3_mode_config(usb3, false, false); 2) Update "usb3->forced_b_device" variable based on connection_state. @@ -2370,6 +2376,7 @@ static void handle_ext_role_switch_states(struct device *dev, usb3_vbus_out(usb3, false); break; case USB_ROLE_DEVICE: + usb3->forced_b_device = true; if (usb3->connection_state == USB_ROLE_NONE) { usb3->connection_state = USB_ROLE_DEVICE; usb3_set_mode(usb3, false); @@ -2384,6 +2391,7 @@ static void handle_ext_role_switch_states(struct device *dev, usb3_vbus_out(usb3, false); break; case USB_ROLE_HOST: + usb3->forced_b_device = false; Can you please confirm are you ok with this changes? Or do you prefer the previous one? Note:- I have done only sanity testing with this changes. Regards, Biju
Hi Biju-san, > From: Biju Das, Sent: Tuesday, April 23, 2019 6:08 PM > > Hi Shimoda-San, > > Thanks for the feedback. > > > Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use > > usb_role_switch framework > > > > Hi Biju-san, > > > > Thank you for the patch! > > > > > From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM > > > > > > RZ/G2E cat874 board is capable of detecting cable connect and > > > disconnect events. Add support for renesas_usb3 to receive connect and > > > disconnect events and support dual-role switch using usb-role-switch > > framework. > > > > > > Signed-off-by: Biju Das <biju.das@bp.renesas.com> > > > --- > > > 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. > > <snip> > > > > > +static void usb3_check_vbus(struct renesas_usb3 *usb3) { > > > + if (usb3->workaround_for_vbus) { > > > + if (usb3->usb_role_switch_property) { > > > + if (usb3->connection_state == USB_ROLE_DEVICE) { > > > + usb3_mode_config(usb3, false, false); > > > > I should have pointed it out the previous version though, why does this > > usb3_mode_config() calling need? > > My guess is: > > - renesas_usb3_start() calls renesas_usb3_init_controller(). > > -- renesas_usb3_init_controller() calls usb3_check_id() and then > > usb_check_vbus(). > > --- usb_check_id() calls usb3_mode_config(usb3, true, true) and then the > > HW acts as host mode. > > ----> So, you'd like the HW to acts as peripheral mode when the > > connection_state is USB_ROLE_DEVICE, > > you added that the usb3_check_vbus() calls usb3_mode_config(usb3, > > false, false). > > > > Is my guess correct? If so, I'd like to add such code into usb3_check_id() like > > below: > > Yes, it is almost correct. The scenario I am trying is > > [1] USB type C cable connected to a Host Machine(TI chip identifies as Device connection. But we haven't installed Gadget > module for Device operation) > > [2] After that trying to install gadget module. In this case, it calls usb_check_id() as mentioned above > and configure it as Host mode. Thank you for the explanation. > > if ((usb3->extcon_host && !usb3->forced_b_device) || > > (usb3->usb_role_switch_property && > > usb3->connection_state == USB_ROLE_HOST)) > > usb3_mode_config(usb3, true, true); > > else > > usb3_mode_config(usb3, false, false); > > > > What do you think? > > Since as per [1], usb3->extcon_host=1 and !usb3->forced_b_device ==1 , The above code always enter into Host Mode > configuration. Oops. Thank you for the pointed it out. > To make it work, I need to update " usb3->forced_b_device" based on connection_state from TI chip. So the new code look > like Since the forced_b_device is related to debug purpose (controlled by debugfs), I don't want to use the value for type-c. > 1) There is no change in usb_check_id() call. > > if (usb3->extcon_host && !usb3->forced_b_device) > usb3_mode_config(usb3, true, true); > else > usb3_mode_config(usb3, false, false); > > 2) Update "usb3->forced_b_device" variable based on connection_state. > > @@ -2370,6 +2376,7 @@ static void handle_ext_role_switch_states(struct device *dev, > usb3_vbus_out(usb3, false); > break; > case USB_ROLE_DEVICE: > + usb3->forced_b_device = true; > if (usb3->connection_state == USB_ROLE_NONE) { > usb3->connection_state = USB_ROLE_DEVICE; > usb3_set_mode(usb3, false); > @@ -2384,6 +2391,7 @@ static void handle_ext_role_switch_states(struct device *dev, > usb3_vbus_out(usb3, false); > break; > case USB_ROLE_HOST: > + usb3->forced_b_device = false; > > Can you please confirm are you ok with this changes? Or do you prefer the previous one? I'd like to change usb3_check_id() somehow. How about the following conditions? In type-c environment, since usb3->usb_role_switch_property is true, it should be OK for it. if ((!usb3->usb_role_switch_property && usb3->extcon_host && !usb3->forced_b_device) || (usb3->usb_role_switch_property && usb3->connection_state == USB_ROLE_HOST)) usb3_mode_config(usb3, true, true); else usb3_mode_config(usb3, false, false); Best regards, Yoshihiro Shimoda > Note:- > I have done only sanity testing with this changes. > > Regards, > Biju > > > > > >
Hi Shimoda-San, Thanks for the feedback. > Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use > usb_role_switch framework > > Hi Biju-san, > > > From: Biju Das, Sent: Tuesday, April 23, 2019 6:08 PM > > > > Hi Shimoda-San, > > > > Thanks for the feedback. > > > > > Subject: RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use > > > usb_role_switch framework > > > > > > Hi Biju-san, > > > > > > Thank you for the patch! > > > > > > > From: Biju Das, Sent: Tuesday, April 16, 2019 6:38 PM > > > > > > > > RZ/G2E cat874 board is capable of detecting cable connect and > > > > disconnect events. Add support for renesas_usb3 to receive connect > > > > and disconnect events and support dual-role switch using > > > > usb-role-switch > > > framework. > > > > > > > > Signed-off-by: Biju Das <biju.das@bp.renesas.com> > > > > --- > > > > 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. > > > <snip> > > > > > > > +static void usb3_check_vbus(struct renesas_usb3 *usb3) { > > > > + if (usb3->workaround_for_vbus) { > > > > + if (usb3->usb_role_switch_property) { > > > > + if (usb3->connection_state == USB_ROLE_DEVICE) { > > > > + usb3_mode_config(usb3, false, false); > > > > > > I should have pointed it out the previous version though, why does > > > this > > > usb3_mode_config() calling need? > > > My guess is: > > > - renesas_usb3_start() calls renesas_usb3_init_controller(). > > > -- renesas_usb3_init_controller() calls usb3_check_id() and then > > > usb_check_vbus(). > > > --- usb_check_id() calls usb3_mode_config(usb3, true, true) and > > > then the HW acts as host mode. > > > ----> So, you'd like the HW to acts as peripheral mode when the > > > connection_state is USB_ROLE_DEVICE, > > > you added that the usb3_check_vbus() calls > > > usb3_mode_config(usb3, false, false). > > > > > > Is my guess correct? If so, I'd like to add such code into > > > usb3_check_id() like > > > below: > > > > Yes, it is almost correct. The scenario I am trying is > > > > [1] USB type C cable connected to a Host Machine(TI chip identifies > > as Device connection. But we haven't installed Gadget module for > > Device operation) > > > > [2] After that trying to install gadget module. In this case, it > > calls usb_check_id() as mentioned above and configure it as Host mode. > > Thank you for the explanation. > > > > if ((usb3->extcon_host && !usb3->forced_b_device) || > > > (usb3->usb_role_switch_property && > > > usb3->connection_state == USB_ROLE_HOST)) > > > usb3_mode_config(usb3, true, true); > > > else > > > usb3_mode_config(usb3, false, false); > > > > > > What do you think? > > > > Since as per [1], usb3->extcon_host=1 and !usb3->forced_b_device ==1 > > , The above code always enter into Host Mode configuration. > > Oops. Thank you for the pointed it out. > > > To make it work, I need to update " usb3->forced_b_device" based on > > connection_state from TI chip. So the new code look like > > Since the forced_b_device is related to debug purpose (controlled by > debugfs), I don't want to use the value for type-c. > > > 1) There is no change in usb_check_id() call. > > > > if (usb3->extcon_host && !usb3->forced_b_device) > > usb3_mode_config(usb3, true, true); > > else > > usb3_mode_config(usb3, false, false); > > > > 2) Update "usb3->forced_b_device" variable based on connection_state. > > > > @@ -2370,6 +2376,7 @@ static void handle_ext_role_switch_states(struct > device *dev, > > usb3_vbus_out(usb3, false); > > break; > > case USB_ROLE_DEVICE: > > + usb3->forced_b_device = true; > > if (usb3->connection_state == USB_ROLE_NONE) { > > usb3->connection_state = USB_ROLE_DEVICE; > > usb3_set_mode(usb3, false); @@ -2384,6 +2391,7 > > @@ static void handle_ext_role_switch_states(struct device *dev, > > usb3_vbus_out(usb3, false); > > break; > > case USB_ROLE_HOST: > > + usb3->forced_b_device = false; > > > > Can you please confirm are you ok with this changes? Or do you prefer the > previous one? > > I'd like to change usb3_check_id() somehow. > How about the following conditions? In type-c environment, since usb3- > >usb_role_switch_property is true, it should be OK for it. > > if ((!usb3->usb_role_switch_property && > usb3->extcon_host && !usb3->forced_b_device) || > (usb3->usb_role_switch_property && > usb3->connection_state == USB_ROLE_HOST)) > usb3_mode_config(usb3, true, true); > else > usb3_mode_config(usb3, false, false); > OK. I will send V5 with this changes. Regards, Biju
diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c index 7dc2485..efee047 100644 --- a/drivers/usb/gadget/udc/renesas_usb3.c +++ b/drivers/usb/gadget/udc/renesas_usb3.c @@ -351,6 +351,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 +361,7 @@ struct renesas_usb3 { bool extcon_usb; /* check vbus and set EXTCON_USB */ bool forced_b_device; bool start_to_connect; + bool usb_role_switch_property; }; #define gadget_to_renesas_usb3(_gadget) \ @@ -644,22 +647,6 @@ static void usb3_disconnect(struct renesas_usb3 *usb3) usb3->driver->disconnect(&usb3->gadget); } -static void usb3_check_vbus(struct renesas_usb3 *usb3) -{ - if (usb3->workaround_for_vbus) { - usb3_connect(usb3); - } else { - usb3->extcon_usb = !!(usb3_read(usb3, USB3_USB_STA) & - USB_STA_VBUS_STA); - if (usb3->extcon_usb) - usb3_connect(usb3); - else - usb3_disconnect(usb3); - - schedule_work(&usb3->extcon_work); - } -} - static void renesas_usb3_role_work(struct work_struct *work) { struct renesas_usb3 *usb3 = @@ -699,8 +686,11 @@ 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->usb_role_switch_property || + 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); @@ -724,6 +714,28 @@ static void usb3_check_id(struct renesas_usb3 *usb3) schedule_work(&usb3->extcon_work); } +static void usb3_check_vbus(struct renesas_usb3 *usb3) +{ + if (usb3->workaround_for_vbus) { + if (usb3->usb_role_switch_property) { + if (usb3->connection_state == USB_ROLE_DEVICE) { + usb3_mode_config(usb3, false, false); + usb3_connect(usb3); + } + } else + usb3_connect(usb3); + } else { + usb3->extcon_usb = !!(usb3_read(usb3, USB3_USB_STA) & + USB_STA_VBUS_STA); + if (usb3->extcon_usb) + usb3_connect(usb3); + else + usb3_disconnect(usb3); + + schedule_work(&usb3->extcon_work); + } +} + static void renesas_usb3_init_controller(struct renesas_usb3 *usb3) { usb3_init_axi_bridge(usb3); @@ -2343,14 +2355,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 +2424,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->usb_role_switch_property) + handle_ext_role_switch_states(dev, role); + else + handle_role_switch_states(dev, role); + pm_runtime_put(dev); return 0; @@ -2650,7 +2727,7 @@ 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, @@ -2741,6 +2818,11 @@ static int renesas_usb3_probe(struct platform_device *pdev) if (ret < 0) goto err_dev_create; + if (device_property_read_bool(&pdev->dev, "renesas,usb-role-switch")) { + usb3->usb_role_switch_property = 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);
RZ/G2E cat874 board is capable of detecting cable connect and disconnect events. Add support for renesas_usb3 to receive connect and disconnect events and support dual-role switch using usb-role-switch framework. Signed-off-by: Biju Das <biju.das@bp.renesas.com> --- 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 | 126 ++++++++++++++++++++++++++++------ 1 file changed, 104 insertions(+), 22 deletions(-)