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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);

Best regards,
Yoshihiro Shimoda

> Note:-
>  I have done only sanity testing with this changes.
> 
> Regards,
> Biju
> 
> 
> 
> 
> 
> 





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux