RE: [PATCH V4 4/7] usb: gadget: udc: renesas_usb3: Use usb_role_switch framework

[Biju Das]

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