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