Re: [PATCH v2 1/2] usb: dwc3: add Realtek DHC RTD SoC dwc3 glue layer driver

[Thinh Nguyen]

On Thu, Aug 03, 2023, Stanley Chang[昌育德] wrote:
> Hi Thinh,
> 
> > > > > +static int dwc3_rtk_setup_role_switch(struct dwc3_rtk *rtk)
> > > >
> > > > Any reason why we're doing the role switch here and not what's
> > > > implemented from the core?
> > > >
> > > Because we have to set the usb 2.0 phy mode through switch_usb2_dr_mode
> > in the function dwc3_rtk_set_dr_mode.
> > > In fact, switch_dwc3_dr_mode will use the role switching implemented by
> > core.
> > >
> > 
> > I don't think this is a good way to go about it. Do you (the glue
> > driver) create a role switch structure and register to the same role switch
> > device that the dwc3 core would register later? I did not expect that to work at
> > all.
> > 
> In our application, this role switch in the glue driver works fine.

Is the order of operation between the switch from the core vs the glue
determinate? Also, which operation should happen first? It's not clear
how you handle it here.

> 
> > How about we create a couple ops callbacks to dwc3 structure. One to
> > initialize gadget and the other for host. Should any platform specific device
> > need some intialization before initializing the specific role, we can invoke
> > these callbacks.
> > 
> > e.g.:
> > 
> > struct dwc3_glue_ops {
> >         int (*init_gadget)(struct dwc3 *dwc);
> >         int (*init_host)(struct dwc3 *dwc); }
> > 
> > struct dwc3 {
> >         ..
> >         const struct dwc3_glue_ops      *ops;
> > }
> > 
> > 
> > static int dwc3_glue_init_gadget(struct dwc3 *dwc) {
> >         if (!dwc->ops->init_gadget)
> >                 return 0;
> > 
> >         return dwc->ops->init_gadget(dwc); }
> > 
> > static int dwc3_glue_init_host(struct dwc3 *dwc) {
> >         if (!dwc->ops->init_host)
> >                 return 0;
> > 
> >         return dwc->ops->init_host(dwc); }
> > 
> > 
> > Your glue driver would implement these callbacks. In dwc3_gadget_init() and
> > dwc3_host_init(), we can call these callbacks at start. I admit that this may not
> > be the greatest solution since it would require the glue driver to alloc and
> > register a dwc3 platform device. But I think should be ok. If anyone else can
> > chime in for more idea, it'd be great.
> 
> 
> Thanks for your advice.
> Here is a solution. As you're concerned, it must register callbacks with the core driver. It looks a bit complicated.
> I noticed the phy_set_mode api.
> Maybe I can try to move switch_usb2_dr_mode to phy driver.
> 
> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
> 

I'm concern if there are other unwanted side effects this may have to
other platforms.

BR,
Thinh