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
