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

[Thinh Nguyen]

On Mon, Aug 07, 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.
> 
> 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.

How do we know that? You're registering to the same switch device,
wouldn't the switch event notify the switch work in both the glue and
the core? Perhaps I'm missing something here, but is there some
mechanism that waits for the switch work from the glue to complete
before the 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://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.4.8/source/drivers/usb/dwc3/core.c*L203__;Iw!!A4F2R9G_pg!dOK1h9Z03CfmtwWlNePk-TiEdHQjaP8qMmY4BgMfYm0yTi5NThLqK_POOzdM89tSz2zDRkfGG9BSzb78zkLFhttxoefU5w$ 
> 

Ah ok, this seems like a better option.

Thanks,
Thinh