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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux