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

[Stanley Chang[昌育德]]

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.

We have a type c driver that first calls role switch in glue.
The role switching function in glue will call the role switching function in core.

Type c driver setting switch role function:
      |----> Set role in the glue
                   |----> Set roles in the core
                   |----> switch_usb2_dr_mode in glue

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


I didn't add any new phy_set_mode to dwc3/core.
I mean use existing callback, as follows
https://elixir.bootlin.com/linux/v6.4.8/source/drivers/usb/dwc3/core.c#L203


Thanks,
Stanley