Re: [RFC v4 3/5] usb: dwc3: core: Do not setup event buffers for host only controllers

[Thinh Nguyen]

On Wed, Jan 18, 2023, Jack Pham wrote:
> Hi Thinh,
> 
> On Thu, Jan 19, 2023 at 12:38:51AM +0000, Thinh Nguyen wrote:
> > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > Multiport controllers being host-only capable do not have GEVNTADDR
> 
> Multiport may not be relevant here.  Host-only is though.
> 
> > > HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event
> > 
> > I think you should reword "present" to something else. They're still
> > present
> 
> In our case we have an instance where the IP is statically configured
> via coreConsultant with DWC_USB31_MODE==1 (host only) and we did observe
> that none of the registers pertaining to device mode (including GEVNT*
> and of course all the D* ones) are even *present* in the register map.
> If we try to access them we encounter some kind of access error or stall
> (or translation fault as described).  So the approach here is to first
> verify by checking the HWPARAMS0 register if the HW is even capable of
> device mode in the first place.

I see.

> 
> > but those registers are to be set while operating in device
> > mode. The rest looks fine.
> 
> Are you suggesting only touching the GEVNT* registers when *operating*
> in device mode, even in the case of a dual-role capable controller?  In
> that case would it make more sense to additionally move the calls to
> dwc3_event_buffers_{setup,cleanup} out of core.c and into
> dwc3_gadget_{init,exit} perhaps?  That way we avoid them completely

While it shouldn't be a problem for DRD, it may be cleaner to do that.

> unless and until we switch into peripheral mode (assuming controller
> supports that, which we should already have checks for).  Moreover, if
> the devicetree dr_mode property is set to host-only we'd also avoid
> calling these.
> 
> > > buffers during core_init can cause an SMMU Fault. Avoid event buffers
> > > setup if the GHWPARAMS0 tells that the controller is host-only.
> > > 
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> > > ---
> > >  drivers/usb/dwc3/core.c | 23 +++++++++++++++--------
> > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 7e0a9a598dfd..f61ebddaecc0 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
> > >  
> > >  static void dwc3_core_exit(struct dwc3 *dwc)
> > >  {
> > > -	int i;
> > > +	int		i;
> > > +	unsigned int	hw_mode;
> > >  
> > > -	dwc3_event_buffers_cleanup(dwc);
> > > +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> > > +	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST)
> 
> If we stick with this approach, we probably could just check
> dwc->dr_mode instead as probe should have already set that to be an
> intersection between the values given in devicetree "dr_mode" and the
> HWPARAMS0 capability.
> 

What we have here should not break DRD, so it's fine either way.

Thanks,
Thinh